dlmanning / gulp-sass

SASS plugin for gulp
MIT License
1.56k stars 381 forks source link

Migrate to use the new Sass JS API by default #846

Open wkillerud opened 2 years ago

wkillerud commented 2 years ago

The legacy render API will be removed in sass 2.0. Keep the legacy API as an option, but default to the new API to encourage adoption.

Add sass-embedded in tests and update assertions to support the results from v2 of the JS API.

Fixes #837

polarbirke commented 2 years ago

Hei William, thanks a lot for pushing this!

I played around with your branch and wasn't able to @use or @import from node_modules like

@import 'bootstrap-sass/assets/stylesheets/bootstrap/variables';

Error in plugin "sass"
Message:
    path/to/my/file.scss
Can't find stylesheet to import.

(it's an old project, don't ask…).

I have passed 'node_modules' as an includePath. Have you by any chance tested a similar use case and figured it out?

wkillerud commented 2 years ago

includePath is part of the legacy API from what I can tell (the one this PR migrates away from). You may need to try loadPaths @polarbirke

If that doesn't work, we use a ~ prefix for node_modules imports in fremtind/jokul and handle that with an importer. For reference, we use patch-package and this patch while waiting for this PR to land.

polarbirke commented 2 years ago

Thanks, that does it! 👍

Unfortunately the compile time I see is still about twice as high when compared to the legacy node-sass setup. Did I overestimate the benefits of this PR / am I missing an additional toggle to gain speed?

wkillerud commented 2 years ago

Speed benefits will probably depend on you also using sass-embedded instead of the regular sass package to compile. Can’t promise any miracles, but it should be a bit faster @polarbirke .

JohnAlbin commented 2 years ago

I just tested this branch with sass-embedded and it compiles .scss files just fine. Sourcemaps also work when using gulp v4's built-in sourcemaps support (not the deprecated gulp-sourcemaps project.)

Not surprisingly, the node-sass-glob-importer didn't work, but I don't think it has been updated to use the new JS API.

wkillerud commented 2 years ago

node-sass-glob-importer is not updated to support the new JS API, no. I looked into[^1] whether the functionality could be ported to the new Importer API, but it seems difficult – especially with the new module system.

One of the goals of @use and @forward in my understanding is for module imports to be more explicit and predictable. If you depend on one or more complex importers for the old syntax I would consider migrating away, or at least not use it for new code.

[^1]: I tried making an importer where canonicalize() returned a new URL("glob:" + url) and used that in load(), but got blocked by not knowing which working directory should be searched by glob. In theory, if you can find the correct directory, you should be able to build the SCSS with @forward and return that from load() as { css, syntax: "scss" }. Proceed with caution, there be dragons 🐉

JohnAlbin commented 2 years ago

I looked into whether the functionality could be ported to the new Importer API, but it seems difficult

Indeed. I only use the globber as a convenience for importing all my mixins with one line instead of one-by-one. If someone wanted to implement a new globber, I think the simpler FileImporter API would be a better fit than the full Importer API.

To be clear, I do not consider a lack of a globber a show stopper for this PR. I'm using this branch now for a new project and hope it will be merged soon.

Thanks for your work, @wkillerud!

neild3r commented 1 year ago

Would love to see this merged. Have tested myself and is working and I am using it succesfully with sass-embedded. Notes are.

sithwarrior commented 2 weeks ago

I just tested this, and it works great, I needed to switch to the newer compile, to get settings such as quiet, quietDeps and silenceDeprecations working, and it works nicely.