Skip to content
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

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wkillerud
Copy link

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

Keep the legacy API around, since its removal is still probably a
while from now.

Fixes dlmanning#837
Add a migration section, link to relevant Sass documentation, and
add a section about still using the legacy API.
@polarbirke
Copy link

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
Copy link
Author

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
Copy link

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
Copy link
Author

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
Copy link

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
Copy link
Author

node-sass-glob-importer is not updated to support the new JS API, no. I looked into1 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.

Footnotes

  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
Copy link

JohnAlbin commented Apr 11, 2022

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
Copy link

neild3r commented Feb 8, 2023

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

  • Importers need to be upgraded to use the new importer API.
  • Sourcemaps seem to use absolute rather than relative paths which can be somewhat mitigated with some sourcemap source modification

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.

Migrate to the new dart-sass JavaScript API
4 participants