Skip to content

dart sass instead of node-sass #2914

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
wants to merge 2 commits into from
Closed

Conversation

simonoff
Copy link

@simonoff simonoff commented Feb 4, 2021

No description provided.

@pedrofurtado
Copy link
Member

Hey, @simonoff ! 👋

Thanks for contribution!

Can you share with us the reason behind the suggestion to switch from node-sass to dart-sass?

Is there some bug related to this change, that dart-sass fix it?

Thank you 🤝 🍻

@simonoff
Copy link
Author

simonoff commented Feb 9, 2021

Hello @pedrofurtado ! Thank you for review.
Generally on the page of node-sass you can see the next info:

Warning: LibSass and Node Sass are deprecated. While they will continue to receive maintenance releases indefinitely, there are no plans to add additional features or compatibility with any new CSS or Sass features. Projects that still use it should move onto Dart Sass.

Secondly right now node-sass requires an installing python to make a node-gyp for arm64 and Node.js 14.x which is required additional time to build docker container on Apple M1.

Also, saas is a drop-in replacement for node-sass. In webpacker 6.x which is not released yet uses a sass right now.

@pedrofurtado
Copy link
Member

@simonoff Thank you so much for feedback! 🤝

I didn't know that node-sass was deprecated. Thanks for notify us!

After your response, I searched and found some articles about it, just like this one:

https://sass-lang.com/blog/libsass-is-deprecated

Is better to move to dart-sass. Pinging @gauravtiwari @guillaumebriday for some more eyes/reviews on it 👓

As I searched, the API exposed by node-sass is the same of dart-sass, to turn the migration more easy. Our tests are all green (except some of them, but is not related with this PR), so I think we are ready to merge ✅

@sam0x17
Copy link

sam0x17 commented Mar 22, 2021

Is there a way to manually use this patch without waiting for the corresponding 5.x release? This is really holding us up. Happy to help however we can.

@mattbrictson
Copy link

Is there a way to manually use this patch without waiting for the corresponding 5.x release? This is really holding us up. Happy to help however we can.

I've manually replaced node-sass with dart sass on webpacker 5.2.1. Maybe this will work for you as well.

Add dart sass to your Rails app's package.json:

$ yarn add sass

Modify config/webpack/environment.js:

["sass", "moduleSass"].forEach((loader) => {
  const sassLoader = environment.loaders
    .get(loader)
    .use.find((el) => el.loader === "sass-loader");
  sassLoader.options.implementation = require("sass");
});

@guillaumebriday
Copy link
Member

LGTM

@sam0x17
Copy link

sam0x17 commented Mar 23, 2021

@mattbrictson thanks so much this got me a lot closer however when I do @use "@material/ripple"; everything builds correctly however the CSS for material does not get added to the payload (curiously there is no warning or error either).

But if I spell the use path incorrectly, like @use "@material/ripp"; I do get an error SassError: Can't find stylesheet to import . So clearly webpack knows about the path, but it just isn't importing the css???

Happy to open a separate issue for this but figured this would be helpful for other people who ended up here

@mattbrictson
Copy link

@mattbrictson thanks so much this got me a lot closer however when I do @use "@material/ripple"; everything builds correctly however the CSS for material does not get added to the payload (curiously there is no warning or error either).

I think this is an unrelated issue. Just guessing, @use is for namespaced access to mixins, functions, etc. By design it does not add CSS to the payload. You probably want @import? https://sass-lang.com/documentation/at-rules/use

@sam0x17
Copy link

sam0x17 commented Mar 23, 2021

@mattbrictson thanks again -- switching to @import had the same results, but I will open a separate issue

update: when using material, apparently the only way to import stuff is by doing @use and then have a corresponding @include. Once I did this everything worked 🎉

@kimyu-ng
Copy link

kimyu-ng commented Apr 8, 2021

can we get this node-sass replacement in for 5.3 along with other security updates? cc @gauravtiwari

@aried3r
Copy link
Contributor

aried3r commented May 20, 2021

I believe this can be closed as it has been released in 5.2.2: https://github.com/rails/webpacker/blob/5-x-stable/CHANGELOG.md#522---2021-04-27.

@guillaumebriday
Copy link
Member

Could you fix conflicts please? @simonoff thanks 🙏

@sam0x17
Copy link

sam0x17 commented Jun 16, 2021

So has it actually been released in 5.2.2?

@SampsonCrowley
Copy link
Contributor

So has it actually been released in 5.2.2?

Yes there are available releases with dart sass

@guillaumebriday
Copy link
Member

It's been done here, thanks @simonoff

@justin808
Copy link
Contributor

Feel free to reopen this PR here:
https://github.com/shakacode/shakapacker/pulls

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.

9 participants