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

Merged
merged 5 commits into from
Nov 27, 2024

Conversation

wkillerud
Copy link
Contributor

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
Contributor 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
Contributor 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
Contributor 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

@sithwarrior
Copy link

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.

@dyaroman
Copy link

@dlmanning Hi, is it possible to merge this PR?

@stof
Copy link
Contributor

stof commented Sep 30, 2024

@dlmanning is there any chance to merge this ? It would solve #870 and #867

@polarbirke
Copy link

Maybe @xzyfer is able to help out here, considering they published the last release in 2021?

@sithwarrior
Copy link

I didn't have that much patience, so I just forked https://github.com/wkillerud/gulp-sass/tree/feat/migrate-to-v2 the code itself is "pretty simple" as its a wrapper that passes the info to sass.

But this should surely be released.

@xzyfer
Copy link
Collaborator

xzyfer commented Sep 30, 2024

Thanks for the tag. I don't actively follow this repo. Will take a look and aim to get a release out this week now that node-sass is deprecated.

@ChaosEngine
Copy link

Any movement in there? Still seeing lot of

Deprecation Warning: The legacy JS API is deprecated and will be removed in Dart Sass 2.0.0.

More info: https://sass-lang.com/d/legacy-js-api

on my build pipeline

@justinkruit
Copy link

@xzyfer Any updates on this?

@ktecho
Copy link

ktecho commented Nov 7, 2024

Hey @dlmanning

Any chance of having this merged? Thanks.

Edit: maybe @xzyfer is at charge now?

@xzyfer xzyfer merged commit 510c1c8 into dlmanning:master Nov 27, 2024
6 of 7 checks passed
@xzyfer
Copy link
Collaborator

xzyfer commented Nov 27, 2024

Thanks for all your effort @wkillerud. I'm very sorry for the delay in getting this released.

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