sass / embedded-host-node

A Node.js library that will communicate with Embedded Dart Sass using the Embedded Sass protocol
MIT License
143 stars 26 forks source link

Rewrite legacy API using containingUrl #309

Closed ntkme closed 2 weeks ago

ntkme commented 1 month ago

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

nex3 commented 1 month ago

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 commented 1 month ago

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.