embedded-host-node icon indicating copy to clipboard operation
embedded-host-node copied to clipboard

Rewrite legacy API using containingUrl

Open ntkme opened this issue 1 year ago • 2 comments

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.

https://github.com/sass/sass/pull/3905 https://github.com/sass/dart-sass/pull/2291

  • Closes #305.

ntkme avatar Jul 27 '24 01:07 ntkme

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.

nex3 avatar Jul 30 '24 22:07 nex3

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.

ntkme avatar Jul 31 '24 15:07 ntkme