sass-loader icon indicating copy to clipboard operation
sass-loader copied to clipboard

Option to use the DartVM executable rather than the pure-js version of dart-sass.

Open jrood opened this issue 5 years ago • 70 comments

  • Operating System: OSX
  • Node Version: 10.16.3
  • NPM Version: 6.11.3
  • webpack Version: 4.39.2
  • sass-loader Version: 7.1.0

Feature Proposal

Currently, the only options for implementation are node-sass and the pure-js version of dart-sass. There should be an option to use the fast DartVM executable version of dart-sass.

The pure JS version is slower than the stand-alone executable (https://sass-lang.com/dart-sass)

Feature Use Case

For projects that have a large codebase of sass files that need to be compiled and performance is important.

jrood avatar Oct 31 '19 16:10 jrood

Make sense, feel free to send a PR

alexander-akait avatar Nov 05 '19 12:11 alexander-akait

https://github.com/sass/embedded-host-node, currently in Alpha, may be the easiest way to achieve this!

rowanbeentje avatar Oct 21 '20 13:10 rowanbeentje

Yep, but right now it is not easy

alexander-akait avatar Oct 21 '20 13:10 alexander-akait

+1!

strarsis avatar Mar 19 '21 10:03 strarsis

@alexander-akait https://github.com/sass/embedded-host-node is getting close to a usable beta that includes custom importers and many other features that the sass-loader relies on. What do you think?

denizsokullu avatar Jan 03 '22 18:01 denizsokullu

Unfortunately, I have not looked at this for a long time, if you have any ideas how we can integrate this now, you can send it, I will be glad any help

alexander-akait avatar Jan 03 '22 19:01 alexander-akait

It will be easy if they have docs :smile: @nex3 Can we try it to integrate and test or is it still very young?

alexander-akait avatar Jan 03 '22 19:01 alexander-akait

As soon as https://github.com/sass/embedded-host-node/pull/94 lands, the embedded host will fully support the new JS API. I'll be cutting a beta release at that point that you can test against.

nex3 avatar Jan 04 '22 21:01 nex3

Just to mention that @nex3 has now merged that PR and cut the beta release: https://github.com/sass/embedded-host-node/releases/tag/1.0.0-beta.8 . I'm excited about this!

rowanbeentje avatar Jan 06 '22 23:01 rowanbeentje

@nex3 Hi, where we can find docs about usage? Thanks

alexander-akait avatar Jan 07 '22 11:01 alexander-akait

It's all documented at https://sass-lang.com/documentation/js-api.

Edit: I realized you probably meant docs specifically about using embedded-host-node. We don't have official docs yet, but it's really just a drop-in replacement for the sass package, so you should be able to add it to your package.json and go from there.

nex3 avatar Jan 07 '22 22:01 nex3

I tried to adopt https://github.com/sass/embedded-host-node locally, and it works good for me, I'll try it here and maybe make a release, perf is almost the same

alexander-akait avatar Jan 08 '22 16:01 alexander-akait

https://github.com/webpack-contrib/sass-loader/pull/1011

alexander-akait avatar Jan 08 '22 16:01 alexander-akait

@nex3 Small feedback:

  1. Function is not work:

Example (if you change sass-embedded on sass/node-sass, all works fine):

var sass = require("sass-embedded");

sass.render(
  {
    data: "#{headings(2,5)} { color: #08c; }\n",
    functions: {
      "headings($from: 0, $to: 6)": (from, to) => {
        const f = from.getValue();
        const t = to.getValue();
        const list = new sass.types.List(t - f + 1);
        let i;

        for (i = f; i <= t; i++) {
          list.setValue(i - f, new sass.types.String(`h${i}`));
        }

        return list;
      },
    },
  },
  function (err, result) {
    if (err) {
      // `err - Error: expected selector.` for `sass-embedded`
      console.log(err);
      
      return;
    }
    console.log(result.css.toString());
  }
);
  1. Importers broken, I tried old format (i.e. just function) and new format (i.e. { canonicalize(url) {}, load(canonicalUrl) {} } ), both is not working, code is simple:
@import import-with-custom-logic

We still use render function to keep compatibility with node-sass/sass (dart), maybe problem with this, I don't found docs about it, but in types they are exists

  1. includePaths doesn't work, we have very simple test where we provide includePaths to sass and just use @import "import-from-included-path";

alexander-akait avatar Jan 08 '22 18:01 alexander-akait

https://github.com/sass/embedded-host-node/blob/main/lib/src/legacy.ts does not seem to have some of the specification included as it masks most of the options before calling the new api compile. @alexander-akait you could try to drop the legacy render method in favor of compileAsync?

denizsokullu avatar Jan 08 '22 19:01 denizsokullu

Yep, we can do it, just interesting why it doesn't work with legacy, because types exist

alexander-akait avatar Jan 10 '22 11:01 alexander-akait

hm, I was wrong, if we change render on compileStringAsync old legacy importers will not work, it is breaking change for us, because many developers still use them...

alexander-akait avatar Jan 10 '22 13:01 alexander-akait

I tried to switch on modern API, but docs are very bad and no examples (spent a lot of time to understand how new API works), so it requires more time than I think...

alexander-akait avatar Jan 10 '22 16:01 alexander-akait

Spent two days on this stuff and no luck, the new importer api is not usable and docs are very bad :disappointed: , https://sass-lang.com/documentation/js-api/interfaces/Importer,

The problem - we don't have prev in new importer API (as we have in old), so we don't know where should we resolve your import/use (you can have @import "variables"; in different files and we should know where it was requested from), we can use the url option, but it will work only for top file and nested imports is no possible to resolve

alexander-akait avatar Jan 10 '22 19:01 alexander-akait

@nex3 Why prev was removed from importers https://sass-lang.com/documentation/js-api/modules#LegacySyncImporter?

Without file where it was requested we can't perform resolving, we don't know context so we don't have where we should start to resolve...

alexander-akait avatar Jan 10 '22 19:01 alexander-akait

Yep, we can do it, just interesting why it doesn't work with legacy, because types exist

We implemented the new API for sass-embedded first. Note that it's currently only a beta release, so it doesn't have everything we plan to add. I'm currently working on adding support for the legacy JS API.

hm, I was wrong, if we change render on compileStringAsync old legacy importers will not work, it is breaking change for us, because many developers still use them...

Correct, old importers won't work with the new JS API and vice versa. Old importers are actually quite difficult to make efficient because they don't play nicely with the concept of "canonical URLs", which was one of the motivations of moving to a new API in the first place.

I tried to switch on modern API, but docs are very bad and no examples (spent a lot of time to understand how new API works), so it requires more time than I think...

I wrote all those docs, so if you can provide specific ways they could be more helpful or additional examples you'd like to see I'd be happy to improve them.

The problem - we don't have prev in new importer API (as we have in old), so we don't know where should we resolve your import/use (you can have @import "variables"; in different files and we should know where it was requested from), we can use the url option, but it will work only for top file and nested imports is no possible to resolve

This would have been a good thing to bring up when we asked for feedback. In this case, though, there's a good reason for the behavior. There are a few invariants we want to make sure all importers abide by, in order to allow the Sass compiler to make important optimizations (like only loading each stylesheet once) and to ensure that users can consistently reason about loads:

  1. There must be a one-to-one mapping between (absolute) canonical URLs and stylesheets. This means that even when a user loads a stylesheet using a relative URL, that stylesheet must have an absolute canonical URL associated with it and loading that canonical URL must return the same stylesheet. This means that any stylesheet can always be loaded using its canonical URL.

  2. Relative URLs are resolved like paths, so for example within scheme:a/b/c.scss the URL ../d should always be resolved to scheme:a/d.

  3. Loads relative to the current stylesheet always take precedence over loads from the load path (including virtual load paths exposed by importers), so if scheme:a/b/x.scss exists then @use "x" within scheme:a/b/c.scss will always load.

The old importer API didn't guarantee any of these invariants. An importer could use the prev parameter to add stylesheets that could only be imported as relative URLs, thus violating (1). Or it could ignore the prev parameter completely and break relative URLs, violating (2) and (3).

Rather than pushing all the complexity of handling these invariants onto importer authors like the old API, the new API is designed so that these invariants can't be violated. To quote the documentation:

Relative loads in stylesheets loaded from an importer are handled by resolving the loaded URL relative to the canonical URL of the stylesheet that contains it, and passing that URL back to the importer's canonicalize method. For example, suppose the "Resolving a Load" example above returned a stylesheet that contained @use "mixins":

  • The compiler resolves the URL mixins relative to the current stylesheet's canonical URL db:foo/bar/baz/_index.scss to get db:foo/bar/baz/mixins.
  • It calls canonicalize with "db:foo/bar/baz/mixins".
  • canonicalize returns new URL("db:foo/bar/baz/_mixins.scss").

Because of this, canonicalize must return a meaningful result when called with a URL relative to one returned by an earlier call to canonicalize.

For your use-case, it's probably easiest to use a FileImporter: this allows you to redirect a Webpack-style load to a specific file, but then automatically handles any relative loads from there without you needing to intervene at all. This is also more efficient, since it saves a round-trip from the compiler for each relative loads.

nex3 avatar Jan 10 '22 22:01 nex3

@nex3

We implemented the new API for sass-embedded first. Note that it's currently only a beta release, so it doesn't have everything we plan to add. I'm currently working on adding support for the legacy JS API.

:+1:

Correct, old importers won't work with the new JS API and vice versa. Old importers are actually quite difficult to make efficient because they don't play nicely with the concept of "canonical URLs", which was one of the motivations of moving to a new API in the first place.

Yep, I noticed this when debug problems, my bad

For your use-case, it's probably easiest to use a FileImporter: this allows you to redirect a Webpack-style load to a specific file, but then automatically handles any relative loads from there without you needing to intervene at all. This is also more efficient, since it saves a round-trip from the compiler for each relative loads.

I am afraid we can't use FileImporter, because we support loading by http/https/file/data and ability to integrate any schema, there are a lot of other places where it can be problem, resolving in webpack fully based context (i.e. prev), without this we can't resolve (like Node.js built-in resolver, ts resolver and etc).

I understand why was it designed this way, but unfortunately this does not give us any opportunity to implement the importer, I want to say it is critical blocker for us.

Yep, I missed it in feedback on new API because a lot of task :disappointed: and to be honest, I didn't even notice that because this is usually the base in any resolving logic...

Unfortunately, all these mean that we cannot integrate new API, to be honestly no one bundler can do it... because the logic in such a place is almost the same for webpack/rollup/vite/etc

alexander-akait avatar Jan 10 '22 22:01 alexander-akait

And FileImporter also doesn't have context (i.e. prev), so resolving is impossible in this case too...

alexander-akait avatar Jan 10 '22 22:01 alexander-akait

Can you give me an example of how webpack's resolution logic uses the file context?

nex3 avatar Jan 10 '22 22:01 nex3

@nex3 Yep, we use it for resolving - https://github.com/webpack/enhanced-resolve, it is supporting all new JS features like exports/imports, type, also plugable and have many built-in plugins, under the hood we use this package here:

simplified logic:

const context = path.dirname(prev);
 
resolve(context, url, (err, result) => {
  result;
});

Just for information - we have more than one resolver for sass, list of them - https://github.com/webpack-contrib/sass-loader/blob/master/src/utils.js#L451

alexander-akait avatar Jan 10 '22 22:01 alexander-akait

What information is needed from the context object that's not available globally? In other words, from a user's perspective, how might @import "~foo" be resolved differently depending on which file it comes from?

nex3 avatar Jan 10 '22 22:01 nex3

@nex3

@import "~foo"

Firstly, aliases - they provided by users, so it can be path on any place, anyway after resolving we have resolved URL, so we can return to sass real path with file:///, there are some note - developer can create plugin which resolve different URLs based on any condition, but it kill webpack cache too

Second, versions - if developer have @import "~foo" inside package inside node_modules, it will be resolved relative to package, because in other package can be @import "~foo" with other version of foo

Thirdly, path can be different based on package manager, yarn/pnpm uses versions in path, i.e. node_modules/.store/[email protected]/style.css (simplified), also yarn supports resoltuions, so requests can be rewritten on package manager level.

Anyway, after resolving we always have path to real file and can convert it to file:///, in most of cases it's deterministic, yep, there are some exceptions, but most often this is problem with configuration.

alexander-akait avatar Jan 10 '22 23:01 alexander-akait

What if we included the canonical URL of the previous file in the options parameter to canonicalize(), but only for non-absolute URLs? That way you could make ~-style loads context-dependent without breaking guarantees for relative loads.

nex3 avatar Jan 10 '22 23:01 nex3

@nex3

What if we included the canonical URL of the previous file in the options parameter to canonicalize(), but only for non-absolute URLs? That way you could make ~-style loads context-dependent without breaking guarantees for relative loads.

Is previous file is file where we have @import/@use?, i.e. I have **style.scss and @use 'variables';, so in importer I need url - variables and /absolute/path/to/style.scss - prev/origin (it can be with file:///)

alexander-akait avatar Jan 11 '22 11:01 alexander-akait

@nex3 Found small problem in modern API, when I have @import "file"; and in file we have broken syntax (or any other errors, for example: a {{}), sass throws an error, in modern API we don't have the file property on Error, in old API we have it, will be great to fix it too, otherwise watching will be broken if developer accidentally do mistake in code

alexander-akait avatar Jan 11 '22 14:01 alexander-akait