meteor-scss icon indicating copy to clipboard operation
meteor-scss copied to clipboard

sass/node-sass -> sass/dart-sass

Open dearlordylord opened this issue 4 years ago • 22 comments

It's not so big of PR now! Works pretty well for me.

Side note: I'm also currently use the code snippet from https://github.com/Meteor-Community-Packages/meteor-scss/pull/258/commits/a4f6b9c54584613d8c7d3c558307d6e52eacb62b#diff-2528c563afd9125ec438980adf8b65ca

      for(const possibleFile of possibleFiles){
        if (possibleFile.match(/\.css$/)) {
          let decodedPath = decodeFilePath(possibleFile, true);
          if (decodedPath && fileExists(decodedPath)) {
            return { absolute: true, path: decodedPath };
          }
        }
      }

to add css handling, would be nice to merge it sometime later : )

dearlordylord avatar Sep 08 '20 12:09 dearlordylord

This PR is here in place of closed now https://github.com/Meteor-Community-Packages/meteor-scss/pull/295

It also works well with the code from https://github.com/Meteor-Community-Packages/meteor-scss/pull/258

dearlordylord avatar Sep 08 '20 12:09 dearlordylord

Sorry for the long delay and thanks a lot. Just tested this on a small project and it looks fine. Honestly I have no idea why the tests are failing (the failures are probably not related to this PR).

@sebakerckhof could you take a look at this? In my opinion it would be a good move to get away from node-sass since dart-sass is the official upstream nowadays.

znerol avatar Oct 02 '20 10:10 znerol

This fails because dart-sass can't handle non-alpahetic characters at the start of an import, which we use for package imports, in line with other meteor css preprocessors. https://github.com/sass/dart-sass/issues/1100

Also note that dart-sass, while faster than the old ruby implementation is still substantially slower than libsass.

sebakerckhof avatar Oct 02 '20 13:10 sebakerckhof

libsass and node-sass are officially deprecated now https://github.com/sass/node-sass/commit/e2391c259167a9692f2c23b0c91caa37502334ca

dearlordylord avatar Nov 01 '20 17:11 dearlordylord

I had a discussion with the dart-sass people about the problem with the non-alphnumeric first character of the import. Long story short, they don't want to / can change it, so we would have to change it. This would be a major breaking change. So I'd like to schedule it together with Meteor 2.0

sebakerckhof avatar Nov 01 '20 20:11 sebakerckhof

FYI: i'm working on a version with dart-sass. A bit more work is required than I thought at first sight and I hit a bug in dart-sass which is a blocker for now. But as soon as the bug is resolved I should have a beta pretty soon. As stated before, the import syntax is not going to be backwards compatible.

sebakerckhof avatar Nov 18 '20 15:11 sebakerckhof

Hey @sebakerckhof. Do you have a branch available that you could push? Not sure if I will be much use, but I could certainly try to help here.

moberegger avatar May 15 '21 15:05 moberegger

@sebakerckhof

Also note that dart-sass, while faster than the old ruby implementation is still substantially slower than libsass.

I saw this note in the README, just in case you weren't aware. No idea how much that will help, though.

Note however that by default, renderSync() is more than twice as fast as render() due to the overhead of asynchronous callbacks. To avoid this performance hit, render() can use the fibers package to call asynchronous importers from the synchronous code path. To enable this, pass the Fiber class to the fiber option:

moberegger avatar May 15 '21 20:05 moberegger

@moberegger Yeah, and since we're using meteor, we can easily use fibers (but maybe not such a great idea seeing how fibers is deprecated). Anyway, performance issues aside, I'm still waiting on these 2 bugs to be fixed in dart-sass before we can have a working version based on dart-sass:

https://github.com/sass/dart-sass/issues/1138 https://github.com/sass/dart-sass/issues/1137

sebakerckhof avatar May 16 '21 12:05 sebakerckhof

@sebakerckhof any updates on this? Really need to import sass from node_modules

kolyasya avatar Nov 23 '21 11:11 kolyasya

Thank you for looking into this! Really appreciate it so far.

I've updated PR to be compatible with current master version, and also bumped sass lib version (checked it as well, it works on my project well)

Looking forward for further news on this one!

I've noticed that bugs sass/dart-sass#1138 sass/dart-sass#1137 are resolved now, which is great news

dearlordylord avatar Dec 14 '21 11:12 dearlordylord

Would that be possible to revisit this PR, since the blocker https://github.com/sass/dart-sass/issues/1137 was resolved? If dart-sass ends up being used in this package in some way or another, it'll increase its business value dramatically 👍

dearlordylord avatar Apr 03 '22 17:04 dearlordylord

Hi there, any ETA for merge?

goyan avatar Nov 16 '22 10:11 goyan

Can people please take this for test. I currently don't have any scss project. I'll merge this for a major release next month if there are no complains.

StorytellerCZ avatar Jan 31 '23 12:01 StorytellerCZ

I have tested this in a decent code base and could not find any issues - both for the local and production build. I think it is a pretty representative project using bootstrap and a bunch of Sass features. While testing, I bumped the sass dependency version to 1.64.2 and did not see any problems either. Since node-sass depends on a rather old Ruby version it is difficult to run this on new Apple silicon Macs (see https://forums.meteor.com/t/solved-fourseven-scss-package-problem-during-installation-node-sass-on-macos/58002/14), thus I think releasing this would help everyone on such a platform!

faburem avatar Aug 07 '23 06:08 faburem

I have tested this in a decent code base and could not find any issues - both for the local and production build. I think it is a pretty representative project using bootstrap and a bunch of Sass features. While testing, I bumped the sass dependency version to 1.64.2 and did not see any problems either. Since node-sass depends on a rather old Ruby version it is difficult to run this on new Apple silicon Macs (see https://forums.meteor.com/t/solved-fourseven-scss-package-problem-during-installation-node-sass-on-macos/58002/14), thus I think releasing this would help everyone on such a platform!

Thank you for looking into it @faburem . Updated version in PR to 1.64.2, hope it'll get merged soon!

dearlordylord avatar Aug 07 '23 08:08 dearlordylord

Since it seems like even after updating to the latest node-sass the main issue which promted this PR won't be fixed I will release this as a major next version.

StorytellerCZ avatar Aug 08 '23 10:08 StorytellerCZ

Since it seems like even after updating to the latest node-sass the main issue which promted this PR won't be fixed I will release this as a major next version.

I think there could be some misunderstanding because this PR does fix the main issue which prompted this PR for me at least, and I successfully use it in my production code. Unless something changed.

https://github.com/Meteor-Community-Packages/meteor-scss/issues/294

dearlordylord avatar Aug 08 '23 10:08 dearlordylord

Sorry I think the confusion comes from my wording "they are deprecated now" in https://github.com/Meteor-Community-Packages/meteor-scss/issues/294#issuecomment-720125498 . What I meant there is that node-sass is deprecated.

dearlordylord avatar Aug 08 '23 10:08 dearlordylord

Hi @Firfi, I tried testing this version on our project, but I'm getting these errors:

=> Errors prevented startup:                  

   While processing files with fourseven:scss (for target web.browser):
   /client/main.scss: Scss compiler error: Error: File to import: ./general not found in file: undefined
   ╷
   2 │ @import './general';
   │         ^^^^^^^^^^^
   ╵
   {}\imports\stylesheets\components.scss 2:9  @import
   {}\client\main.scss 7:9                     root stylesheet

   error: Scss compiler error: Error: File to import: ./general not found in file: undefined
   ╷
   2 │ @import './general';
   │         ^^^^^^^^^^^
   ╵
   {}\imports\stylesheets\components.scss 2:9  @import
   {}\client\main.scss 7:9                     root stylesheet


=> Your application has errors. Waiting for file change.

I tried changing it to just 'general' and even 'general.scss' './general.scss', but no luck.

I'm not sure what causes it (path issues, maybe?)

When I reverted to the latest version, it started working again.

Tested using Meteor 2.12, both on Windows 11 and Mac OS 13.0 (Intel)

dsmalicsi avatar Aug 09 '23 09:08 dsmalicsi

Hi @Firfi, I tried testing this version on our project, but I'm getting these errors:

=> Errors prevented startup:                  

   While processing files with fourseven:scss (for target web.browser):
   /client/main.scss: Scss compiler error: Error: File to import: ./general not found in file: undefined
   ╷
   2 │ @import './general';
   │         ^^^^^^^^^^^
   ╵
   {}\imports\stylesheets\components.scss 2:9  @import
   {}\client\main.scss 7:9                     root stylesheet

   error: Scss compiler error: Error: File to import: ./general not found in file: undefined
   ╷
   2 │ @import './general';
   │         ^^^^^^^^^^^
   ╵
   {}\imports\stylesheets\components.scss 2:9  @import
   {}\client\main.scss 7:9                     root stylesheet


=> Your application has errors. Waiting for file change.

I tried changing it to just 'general' and even 'general.scss' './general.scss', but no luck.

I'm not sure what causes it (path issues, maybe?)

When I reverted to the latest version, it started working again.

Tested using Meteor 2.12, both on Windows 11 and Mac OS 13.0 (Intel)

Is it possible for you to share the project or a minimal reproduction? I can try to look into it @dsmalicsi

dearlordylord avatar Aug 09 '23 16:08 dearlordylord