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

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

Closed
jrood opened this issue Oct 31, 2019 · 73 comments · Fixed by #1197
Closed

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

jrood opened this issue Oct 31, 2019 · 73 comments · Fixed by #1197

Comments

@jrood
Copy link

jrood commented Oct 31, 2019

  • 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.

@alexander-akait
Copy link
Member

Make sense, feel free to send a PR

@rowanbeentje
Copy link

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

@alexander-akait
Copy link
Member

Yep, but right now it is not easy

@strarsis
Copy link

+1!

@denizsokullu
Copy link

@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?

@alexander-akait
Copy link
Member

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
Copy link
Member

alexander-akait commented Jan 3, 2022

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

@nex3
Copy link
Contributor

nex3 commented Jan 4, 2022

As soon as sass/embedded-host-node#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.

@rowanbeentje
Copy link

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!

@alexander-akait
Copy link
Member

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

@nex3
Copy link
Contributor

nex3 commented Jan 7, 2022

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.

@alexander-akait
Copy link
Member

alexander-akait commented Jan 8, 2022

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
Copy link
Member

#1011

@alexander-akait
Copy link
Member

alexander-akait commented Jan 8, 2022

@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";

@denizsokullu
Copy link

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?

@alexander-akait
Copy link
Member

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

@alexander-akait
Copy link
Member

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
Copy link
Member

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
Copy link
Member

alexander-akait commented Jan 10, 2022

Spent two days on this stuff and no luck, the new importer api is not usable and docs are very bad 😞 , 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
Copy link
Member

alexander-akait commented Jan 10, 2022

@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...

@nex3
Copy link
Contributor

nex3 commented Jan 10, 2022

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.

@jmbastidas
Copy link

jmbastidas commented Mar 22, 2022

@nex3 hello, do you have news on this?

@alexander-akait
Copy link
Member

Looks like fixed, I will test

@alexandernst
Copy link

Can you point to the commit which fixed this, please?

@alexander-akait
Copy link
Member

All tests passed #1044

@Clarkkkk
Copy link

I want to do release today, there are some bugs with source maps and @use (edge case) in sass-embedded (I will ping nex3 here and report about them here, we can't fix them in sass-loader), also I implement api: "modern" | "legacy", but it is experimental and currently modern API doesn't support built-in webpack resolver, i.e. all styles from node_modules will be not loaded, but you can use loadPaths if you have simple project, if you rely on the sass field in package.json/aliases or somethings more complex it will not work with modern API, unfortunately to solve it we need to improve some things on sass side.

Benchmarks (locally, bootstrap 5 latest):

node-sass#render x 2.70 ops/sec ±1.36% (18 runs sampled)
dart-sass#render x 0.41 ops/sec ±12.03% (7 runs sampled)
sass-embedded#render x 3.95 ops/sec ±0.74% (24 runs sampled)
Fastest is sass-embedded#render

I think words are unnecessary here

I simply add implemetation: 'embedded-sass' and saw a speed regression (build time from 1min 12s to 1min 40s). Did I miss something?

Both sass-loader and embedded-sass are latest version.
Device: M1 Mac mini
OS: Mac OS 12.3.1
Node: 16.11.1

@alexander-akait
Copy link
Member

@Clarkkkk hard to say, how you profile?

@Clarkkkk
Copy link

Clarkkkk commented May 9, 2022

@Clarkkkk hard to say, how you profile?

The config is like:

    entry: './src/index.js',
    mode: 'production',
    output: {
        filename: 'main.js',
        path: path.resolve(__dirname, 'dist'),
    },
    module: {
        rules: [{
            test: /\.(sa|sc)ss$/,
            use: [
                {
                    loader: MiniCssExtractPlugin.loader
                },
                {
                    loader: 'css-loader',
                    options: {
                        modules: {
                            exportLocalsConvention: 'camelCase'
                        },
                    }
                },
                'postcss-loader',
                {
                    loader: 'sass-loader',
                    options: {
                        sassOptions: {
                            charset: false
                        },
                        implementation: require('sass-embedded')
                    }
                }
            ]
        }]
    },
    plugins: [
        new MiniCssExtractPlugin(),
        new CleanWebpackPlugin(),
    ],

But when I use the same config on a simple bootstrap example, sass-embedded is indeed faster. It is not easy to reproduce the speed regression issue.

@alexander-akait
Copy link
Member

@Clarkkkk maybe you can profile, I file loke regression on sass-embedded side

@Clarkkkk
Copy link

@Clarkkkk maybe you can profile, I file loke regression on sass-embedded side

I'm sorry, but what do you mean 'profile'?

@alexander-akait
Copy link
Member

Profile code and undestand where it you have perf problems, https://nodejs.org/en/docs/guides/simple-profiling/

@Clarkkkk
Copy link

Oh, I see.

The original log file is quite large (over 100 M), here is the processed txt file: https://1drv.ms/t/s!AqaKW1-15JGra_rU4o09K44A04E. (It generates 12 files after compilation, and this is one that includes sass-embedded. If you need the others, I can provide them as well)

@alexander-akait
Copy link
Member

@Clarkkkk I think better to open an issue in sass-embedded repo, we can't fix it here

@AprilArcus
Copy link

I happened to give sass-embedded a try, and found that performance actually degraded by a small but statistically significant amount. Results are with an empty cache and four thread-loader workers.

mean 95% CI
JS (sass) 63.6s ± 0.3s
AOT (sass-embedded) 66.1s ± 0.4s

I was surprised by this based on the results other folks have been reporting. Please let me know if you'd like me to do more benchmarking or profiling; I'm happy to help if I can.

@nex3
Copy link
Contributor

nex3 commented Aug 23, 2022

@AprilArcus That's expected if you're compiling many small CSS files. See sass/sass#3296.

@tannera
Copy link

tannera commented Sep 6, 2022

@AprilArcus That's expected if you're compiling many small CSS files. See sass/sass#3296.

Unfortunately it's a common use case to have many small sass files for those using CSS Modules in a large React codebase.

@davidmurdoch
Copy link

I'm using the latest sass-loader and sass packages in webpack 5 and this works:

  sassOptions: {
    api: "legacy",
    webpackImporter: false,
    implementation: sass,
    includePaths: [
      'ui/css',
      'node_modules'
    ],
  },

but this throws an error:

  sassOptions: {
    api: "modern",
    webpackImporter: false,
    implementation: sass,
    loadPaths: [
      'ui/css',
      'node_modules'
    ],
  },

The error:

ERROR in ./ui/css/index.scss
  Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
  Can't find stylesheet to import.
    ╷
  3 │ @use "design-system";
    │ ^^^^^^^^^^^^^^^^^^^^
    ╵
    ui/components/component-library/box/box.scss 3:1                        @import
    ui/components/component-library/component-library-components.scss 14:9  @import
    ui/css/index.scss 13:9                                                  root stylesheet
   @ ./app/home.html 1:250-315

I also tried using sass directly from the command line and it works fine with the modern api:

npx sass@latest ./ui/css/index.scss ./dist/index.css --load-path ./node_modules --load-path ./ui/css

I also tried setting the paths in loadPaths to absolute paths, but I just get the same error.

What is the difference between sass-loader's implementation of the legacy api's includePaths and the modern api's loadPaths?

@evenstensberg
Copy link
Member

@davidmurdoch could you create a reproduction repo. I'll have a look.

@alexander-akait
Copy link
Member

Looks like most of tests passed - #1197, will finish soon and make a release

@alexander-akait
Copy link
Member

Also using sass-embedded and modern-compiler API (was implemented recently) will decrease sass/scss compilation time around 5-10x

@alexander-akait
Copy link
Member

alexander-akait commented Apr 11, 2024

All tests passed, please try https://github.com/webpack-contrib/sass-loader/releases/tag/v14.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.