Skip to content

node-sass -> dart-sass #323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

illusionfield
Copy link

@illusionfield illusionfield commented Jan 24, 2025

Closing this PR in favor of the consolidated version here:
➡️ #329 – Finalize Dart Sass migration & drop direct dependency

That PR targets release/5.0.0 and includes all changes from this one, along with updated docs and migration instructions.
Looking forward to your feedback there!

Hi,

I've spent quite some time working on the integration of Meteor JS and Dart Sass. Given how things are evolving with Dart Sass, I wanted to share my approach. I'm really curious to hear the feedback from the maintainer community on whether this direction is viable.

  • It requires Meteor 3
  • Dropped the seba:minifiers-autoprefixer dependency and aligned it with Meteor’s standard-minifier, which now generates perfect mappings.
  • Reworked the scss-config.json for improved handling.
  • The includePaths config option seems less relevant now, but this might need further review to cover any cases I might have missed.
  • Due to unique import strategies in both Meteor and Dart Sass, I had to sync up the import functionality, so it’s not fully backward-compatible. Dart Sass's importer returns a URL object, while its input is an URL string, which would have broken the @import "{my-package:pretty-buttons}/..."; implementation.

Copy link
Member

@StorytellerCZ StorytellerCZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need more documentation on migration. Other than that I'm happy to move this forward.

Copy link
Member

@jankapunkt jankapunkt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would definitely move forward with this but I would publish an alpha version first, so we can field test this with some real-world apps.

@illusionfield
Copy link
Author

The issue with Meteor 2.8 compatibility was that when I published the package with a Meteor 3.x release, it wasn’t compatible with fibers in a 2.8 installation. On the other hand, if I published it with a 2.8 release, it couldn’t find fibers in installations above 3.0.

This makes sense given the structural changes in Meteor 3, but I couldn’t find a real solution to make it work seamlessly across both versions. That’s why I set the minimum supported Meteor version to 2.10.

I found more details about this issue here:
https://forums.meteor.com/t/solved-is-it-possible-to-publish-a-meteor-compiler-package-that-works-for-both-2-0-and-3-0-yes

@illusionfield illusionfield changed the base branch from master to release/5 February 7, 2025 16:50
@illusionfield illusionfield changed the base branch from release/5 to master February 7, 2025 17:23
@StorytellerCZ
Copy link
Member

@jankapunkt now that @babel/runtime has been remove, I think we can move to releasing a beta and move this forward to full and proper release. We will ask people to test and give feedback again on Friday and then let's move to final release quickly.

@DblK
Copy link

DblK commented Feb 14, 2025

I was looking to move to the next version to avoid issue with install of python for node-sass, but one key feature is missing includePaths.
Do you plan to add it back for final 5.0.0 version?

@illusionfield
Copy link
Author

@DblK

This feature has theoretically lost its purpose in 5.0 since predefined search paths are no longer necessary for relative imports.

The @import (or now @use) works in multiple ways:

  • Relative paths: ../../example
  • Project absolute path: '{}/imports/...'
  • Node modules: ~pkg (resolves to project/node_modules/pkg)
  • Absolute file system paths: "/home/user/dev/globalstyles" or "C:\Styles\fonts"

However, if there are specific cases where includePaths is still needed, let’s discuss it. There might be some edge cases I haven’t considered.

@DblK
Copy link

DblK commented Feb 14, 2025

I do understand there are some workaround and I can manage this, however I have one file called _mixins.scss and I have dozen of scss refering to it by @import "mixins" and my includePaths set to ['{}/client'].
The only way to make it work is to update all the scss files to @import "{}/client/mixins" (or better use @use for better compatibility for the next version.)

I just wanted to avoid the use of {}/client/ in every single scss file. includePaths was very helpful for that. But if there is no other way..

And I am lucky if my includePaths was ['{}/some/very/long/path/for/holding/my/mixins/folder'], it would have been more painful to add it in every single scss file 😅.

@illusionfield
Copy link
Author

I see your point, and I totally understand why includePaths was useful in that case.

A good alternative is using @forward and @use, which are the recommended approaches in modern Sass versions. Instead of relying on includePaths, you can create a central file (e.g., client/mixins/_index.scss) and forward your mixins like this:

@forward "{}/some/very/long/path/for/holding/my/mixins/folder";

Then, in your SCSS files, instead of @import "mixins", you can simply use:

@use "{}/client/mixins" as *;

This way, all your mixins and variables are available everywhere without having to update each file individually. It’s more modular and keeps your paths clean while ensuring better compatibility with future Sass versions.

I hope I understood your issue correctly, but if not, let’s discuss it further! 😊

Maybe it's worth considering a hot prefix for "{}/client/" and "{}/imports/", similar to how ~ works for "{}/node_modules/", as an alternative to includePaths.

@StorytellerCZ, what do you think about this?

Whichever direction we take, I believe this feature discussion should be moved to a separate issue.

@StorytellerCZ
Copy link
Member

Don't really have opinion here. Since we are doing a major version upgrade it is allowed to break some things, just need to ensure that it is properly documented and mentioned in the migration docs.
If it is not much hassle easing the transition by this is fine with me, but then I would be for that we highlight it as such and that people shouldn't expect this to work in the next major release.

@dr-dimitru
Copy link

@illusionfield I agree with @StorytellerCZ that major bump will require detailed migration guidelines and/or references to "modern" SCSS/SASS practices, especially on including/importing files

@illusionfield
Copy link
Author

@dr-dimitru
I agree, and I've already started working on a draft version (here). It looks like some additional adjustments will be needed, especially regarding the includePaths issue.

I wouldn’t rule out the possibility of it being merged back soon, but there's also a critical bug that just came up (here), so there's still some work to do to ensure everything is in good shape for everyone.

@dr-dimitru
Copy link

@illusionfield I left comments for migration doc and changes in readme

Copy link

@dr-dimitru dr-dimitru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comments regarding documentation

@jankapunkt
Copy link
Member

@illusionfield just let me know when there is a beta release to be published, I will then review and publish.

@illusionfield
Copy link
Author

@illusionfield just let me know when there is a beta release to be published, I will then review and publish.

@dr-dimitru when do you think the documentation is likely to be ready?
Thanks again for your help! 🙏🙏🙏

@illusionfield illusionfield requested a review from dr-dimitru March 6, 2025 12:38
@illusionfield
Copy link
Author

I have updated the documentation—it's not necessarily perfect, but should be a good improvement. I have also fixed all issues related to the 5.0 release. The current version is 5.0.0-alpha.3.

@jankapunkt I believe the package is ready for the beta release.

@jankapunkt
Copy link
Member

I released 5.0.0-alpha.3 as this version was set in package.js but there was no release for it. Wen treat it like a beat though :-D

@illusionfield
Copy link
Author

I hope the PR can be merged soon, as I would like to switch back from the temporary alternative to the official version in my projects. 😃

@illusionfield
Copy link
Author

I took some inspiration from here: veliovgroup/flow-router#123
Hopefully, this will only make the package even stronger.

@illusionfield
Copy link
Author

Hi everyone,

Some time has passed, and the latest release seems to be working fine.

@dr-dimitru, @StorytellerCZ Do you have any feedback or observations regarding the documentation?

@jankapunkt If the documentation is also in good shape, then maybe this could be rc.0 instead of beta.
What would be the next steps to get this PR merged into the main branch as soon as possible?

Additionally, I have a question:
I have an advanced development version that no longer includes Sass as an npm dependency but allows it to be installed separately in node_modules. This is important because I use Dart Sass Embedded in my projects.
How would it be best to integrate this as an official option so it can evolve alongside this package if needed?

@jankapunkt
Copy link
Member

jankapunkt commented Mar 12, 2025

@jankapunkt If the documentation is also in good shape, then maybe this could be rc.0 instead of beta.

no problem

What would be the next steps to get this PR merged into the main branch as soon as possible?

We first need to make sure we have at least one or two more people testing this as part of their running apps. I have two mid-sized apps with bootstrap and some custom SCSS and would like to check this out. However, I'm currently occupied with some other things at work.
Therefore I kindly ask @dr-dimitru and @StorytellerCZ to do some testing with their apps, if possible.
I just want to avoid this getting merged too soon and then incoming issue reports building immediate pressure (which I will not be able to handle then)

Additionally, I have a question: I have an advanced development version that no longer includes Sass as an npm dependency but allows it to be installed separately in node_modules. This is important because I use Dart Sass Embedded in my projects. How would it be best to integrate this as an official option so it can evolve alongside this package if needed?

How is this integrated within your project? Simply by installing the package or by some "linking"? If you need to link this somewhere then I would go for a dependency-injection.

@illusionfield
Copy link
Author

illusionfield commented Mar 12, 2025

@jankapunkt
I must confess—I "borrowed" the idea from an official Meteor package 😄
standard-minifier-css.

Right now, it's still for private use, and I'm evaluating whether it makes sense to proceed in this direction.

I'm already using it in a few projects, and it works—but of course, that doesn't necessarily mean it's suitable for the official community package. That still needs to be determined.

@dr-dimitru
Copy link

@jankapunkt If the documentation is also in good shape, then maybe this could be rc.0 instead of beta.

no problem

I'll review/push changes sometime soon. Wrapping up now with my packages that await for update by community

What would be the next steps to get this PR merged into the main branch as soon as possible?

We first need to make sure we have at least one or two more people testing this as part of their running apps. I have two mid-sized apps with bootstrap and some custom SCSS and would like to check this out. However, I'm currently occupied with some other things at work. Therefore I kindly ask @dr-dimitru and @StorytellerCZ to do some testing with their apps, if possible. I just want to avoid this getting merged too soon and then incoming issue reports building immediate pressure (which I will not be able to handle then)

No problem, will test and report here soon

Additionally, I have a question: I have an advanced development version that no longer includes Sass as an npm dependency but allows it to be installed separately in node_modules. This is important because I use Dart Sass Embedded in my projects. How would it be best to integrate this as an official option so it can evolve alongside this package if needed?

How is this integrated within your project? Simply by installing the package or by some "linking"? If you need to link this somewhere then I would go for a dependency-injection.

@illusionfield Same question here, if it will be solved with simple install development npm package X in the docs — It's okay, if it's more than simply installing a package — I'd link it as dependency

@illusionfield
Copy link
Author

illusionfield commented Mar 13, 2025

@dr-dimitru

The integration is as simple as running:

meteor npm install –save-dev sass

or

meteor npm install –save-dev sass-embedded

No additional linking or special setup is required.

There are multiple reasons for this approach

  • Our company does not use a dedicated UI framework (like Bootstrap, etc.), but instead, we have a custom framework built directly on raw Sass. Because of this, we pay extra attention to how Sass is handled in our workflow.

  • Dart Sass Embedded is significantly faster than regular Dart Sass, but it is not platform-independent. Even node-gyp doesn’t solve this because it handles platform dependencies via optionalDependencies (reference here).
    This means bundling it into the Meteor build is not feasible since publish-for-arch is no longer available.

  • The quietDeps option in Sass only mutes warnings for dependencies (see docs).

    • In version 1.79, it did not include @import deprecation warnings.
    • However, in ^1.80, @import deprecation warnings were introduced.
    • In our older projects where we haven’t yet migrated @import to @use or @forward, we temporarily force version 1.79 to avoid excessive console warnings while focusing on refactoring.

Addressed an issue where running `meteor test-packages` in Meteor 2 failed due to npm dependency installation errors related to the `meteor-node-stubs` update. This fix ensures compatibility and resolves the 'npmignore: command not found' error during package tests.

For more details, see: meteor/meteor#13691
- Removed `sass` as a direct dependency to allow full control over Sass versioning
- Enables usage of both `sass` and `sass-embedded` via devDependencies
- Updated README.md for improved clarity, examples, and migration guidance
- Bumped version to `5.1.0-beta.1` to reflect breaking internal change
@illusionfield illusionfield mentioned this pull request May 12, 2025
@illusionfield
Copy link
Author

Closing this PR in favor of the consolidated version here:
➡️ #329 – Finalize Dart Sass migration & drop direct dependency

That PR targets release/5.0.0 and includes all changes from this one, along with updated docs and migration instructions.
Looking forward to your feedback there!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants