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

Rewrite legacy API using containingUrl #309

Closed
wants to merge 1 commit into from
Closed

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Jul 27, 2024

This PR ports the strategy used by https://github.com/sass-contrib/sassc-embedded-shim-ruby to emulate SassC Ruby API and rewrites how legacy JS API is emulated.

The key is to use containingUrl to track the loading stack, and I'm able to get rid of significant amount of hacks in the previous implementation. In addition, it uses a pair of Importer and FileImporter, so that when LegacyImporter returns a file path, it's handled via FileImporter as a performance optimization to avoid reading large files on host side.

By the way, although I couldn't figure out what's wrong with #305 in the previous implementation, I did confirm locally that this reimplementation fixes that issue.

sass/sass#3905
sass/dart-sass#2291

@nex3
Copy link
Contributor

nex3 commented Jul 30, 2024

I believe this would break the ability of legacy importers to fully override file: URLs to do their own thing. For example:

let count = 1;
console.log(sass.renderSync({
  data: '@import "file:foo"; @import "file:foo"',
  importer: function(url, prev) {
    console.log(url);
    return {contents: `foo {count: ${count++}}`}
  }
}).css.toString());

I'm also a little wary of dramatically changing a (mostly) stable implementation this close to the release of Dart Sass 2.0.0, where we're going to remove it entirely.

@ntkme
Copy link
Contributor Author

ntkme commented Jul 31, 2024

I believe this would break the ability of legacy importers to fully override file: URLs to do their own thing.

Looks like this is already broken in the existing legacy implementation without this PR: the sass package will print count: 1 and count: 2, but sass-embedded will print count: 1 and count: 1.

I pushed another tiny change, and now with this PR it would print count: 1 and count: 2 just like sass.

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.

sass-embedded error with bootstrap 5.3.3
2 participants