search

sass / dart-sass

The reference implementation of Sass, written in Dart.
https://sass-lang.com/dart-sass
MIT License
3.97k stars 360 forks source link

sass-loader additionalData broken since Sass 1.43.2 #1540

Closed cdoauthtest closed 2 years ago

cdoauthtest commented 3 years ago

works normally in Sass 1.43.0, but the new version reports an error.

error

SassError: Can't find stylesheet to import.
1 │ @import 'sass/helpers.scss';
       ^^^^^^^^^^^^^^^^^^

webpack.config.js

const srcPath = path.resolve(__dirname, './src');
module.exports = {
  context: srcPath,
  module: {
    rules: [
      {
        test: /\.scss$/,
        use: [
          ...
          {
            loader: 'sass-loader',
            options: {
              additionalData: `@import 'src/sass/helpers.scss';`,
            },
          },
        ],
      },
    ],
  },
  resolve: {
    modules: [srcPath],
  },
};

file structure

├── src
│   ├── sass
│       └── helpers.scss
└── webpack.config.js
stof commented 3 years ago

This should be reported to the webpack sass-loader, not to dart-sass, as this additionalData option is a sass-loader option, not an option of the sass compiler.

sass-loader is maintained at https://github.com/webpack-contrib/sass-loader by a different team.

cdoauthtest commented 3 years ago

This should be reported to the webpack sass-loader, not to dart-sass, as this additionalData option is a sass-loader option, not an option of the sass compiler.

sass-loader is maintained at https://github.com/webpack-contrib/sass-loader by a different team.

https://github.com/webpack-contrib/sass-loader/issues/994#issuecomment-955198946

the maintainer of sass-Loader told me to reported the bug here...

stof commented 3 years ago

Well, I don't understand why they asked that, as it is about their option, so nothing that can be reproduced with sass directly.

stof commented 3 years ago

btw, without a reproducer, it will be hard to help.

One weird thing I see is that the @import rule in your error message is not the same than the one in your additionalData option. So maybe the error is not even related to that option.

cdoauthtest commented 3 years ago

btw, without a reproducer, it will be hard to help.

One weird thing I see is that the @import rule in your error message is not the same than the one in your additionalData option. So maybe the error is not even related to that option.

I copied the wrong code in the example above, the webpack configuration that will report errors is as follows

const srcPath = path.resolve(__dirname, './src');
module.exports = {
  context: srcPath,
  module: {
    rules: [
      {
        test: /\.scss$/,
        use: [
          ...
          {
            loader: 'sass-loader',
            options: {
              additionalData: `@import 'sass/helpers.scss';`,
            },
          },
        ],
      },
    ],
  },
  resolve: {
    modules: [srcPath],
  },
};
stof commented 3 years ago

Even then, I don't think this is a full reproducer. A full reproducer is something that someone can run locally to debug things, without having to guess any missing part.

Levivb commented 3 years ago

Hey, Jumping in here since I've noticed a rather similar thing on my end.

I've been debugging for a couple of hours now and I've been able to pinpoint that the sole upgrade of sass from 1.42.1 to 1.43.2 causes the import of a file to fail which should be reachable through an added resolver modules path .

Let me explain.

$ npm --version
node --v8.0.0

$ node --version
v16.11.1

$ mix --version # using laravel-mix
6.0.11

The webpack config:

{
  context: '/path/to/project',
  mode: 'production',
  infrastructureLogging: {},
  entry: {
    mix: [
      '/path/to/project/node_modules/laravel-mix/src/builder/mock-entry.js',
      '/path/to/project/vendor/laravel-modules/component/src/resources/assets/admin/sass/main.scss',
      '/path/to/project/vendor/laravel-modules/core/src/resources/assets/admin/sass/main.scss',
      '/path/to/project/vendor/laravel-modules/core/src/resources/assets/admin/js/v1/plugin/ckeditor/skins/rudder/editor.scss',
      '/path/to/project/vendor/laravel-modules/core/src/resources/assets/admin/js/v1/plugin/ckeditor/skins/rudder/dialog.scss',
      '/path/to/project/vendor/laravel-modules/media/src/resources/assets/admin/sass/main.scss',
      '/path/to/project/vendor/laravel-modules/page/src/resources/assets/admin/sass/main.scss',
      '/path/to/project/vendor/laravel-modules/routing/src/resources/assets/admin/sass/main.scss'
    ]
  },
  output: {
    path: '/path/to/project/public',
    filename: '[name].js',
    chunkFilename: [Function: chunkFilename],
    publicPath: '/'
  },
  module: {
    rules: [
      [Object], [Object],
      [Object], [Object],
      [Object], [Object],
      [Object], [Object],
      [Object], [Object],
      [Object], [Object],
      [Object], [Object],
      [Object], [Object]
    ]
  },
  plugins: [
    MockEntryPlugin {},
    DefinePlugin { definitions: [Object] },
    CustomTasksPlugin {},
    BuildCallbackPlugin { callback: [Function (anonymous)] },
    BuildOutputPlugin { options: [Object], patched: false },
    MiniCssExtractPlugin {
      options: [Object],
      runtimeOptions: [Object]
    },
    WebpackNotifierPlugin {
      options: [Object],
      lastBuildSucceeded: false,
      isFirstBuild: true
    },
    ProvidePlugin { definitions: [Object] },
    {},
    SourceMapDevToolPlugin {
      sourceMapFilename: '[file].map',
      sourceMappingURLComment: '\n//# sourceMappingURL=[url]',
      moduleFilenameTemplate: 'webpack://[namespace]/[resourcePath]',
      fallbackModuleFilenameTemplate: 'webpack://[namespace]/[resourcePath]?[hash]',
      namespace: '',
      options: [Object]
    }
  ],
  resolve: {
    extensions: [ '*', '.wasm', '.mjs', '.js', '.jsx', '.json' ],
    roots: [ '/path/to/project/public' ],
    modules: [
      'node_modules',
      '/path/to/project/vendor/laravel-modules/core/src/resources/assets/admin/sass',
      '/path/to/project/public/build/prepare/modules',
      '/path/to/project/public/build/prepare/app'
    ]
  },
  stats: { preset: 'errors-warnings', performance: true, timings: true },
  performance: { hints: false },
  optimization: {
    providedExports: true,
    sideEffects: true,
    usedExports: true,
    minimizer: [ [TerserPlugin] ],
    splitChunks: { cacheGroups: [Object] }
  },
  devtool: false,
  devServer: {
    headers: { 'Access-Control-Allow-Origin': '*' },
    static: '/path/to/project/public',
    historyApiFallback: true,
    compress: true,
    firewall: false
  },
  watchOptions: { ignored: /node_modules/ }
}

Notice the: resolve.modules array which adds a few additional directories for sass to lookup files to import.

The main.scss (this is the full file):

@import "~sass/icon";

and a file list of /path/to/project/public/build/prepare/app director (which is added to the resolver.modules configuration) shows:

$ ls -alR public/build/prepare/app/
drwxr-xr-x - Levi  7 Nov 00:33 sass

public/build/prepare/app/sass:
.rw-r--r-- 555 Levi  7 Nov 01:10 _icon.scss

Notice how _icon.scss exists.

First, the working npm run cmd run with sass@1.42.1

npm ls gives the following output:

├── argv@0.0.2
├── babel-eslint@10.1.0
├── browser-sync-webpack-plugin@2.3.0
├── browser-sync@2.26.14
├── dotenv@6.2.0
├── eslint-config-airbnb-base@14.2.1
├── eslint-plugin-import@2.22.1
├── eslint@7.19.0
├── instant.page@5.1.0
├── jquery@3.5.1
├── laravel-mix-merge-manifest@2.0.0
├── laravel-mix@6.0.11
├── npm-check@5.9.2
├── postcss@8.2.6
├── sass-loader@8.0.2
├── sass@1.42.1
├── svg-sprite@1.5.0
└── tailwindcss@2.0.3

and the run command:

$ npm run prod:app-combine

> prod:app-combine
> mix --production -- --env --section=app

Browserslist: caniuse-lite is outdated. Please run:
npx browserslist@latest --update-db

Why you should do it regularly:
https://github.com/browserslist/browserslist#browsers-data-updating
99% done plugins BuildOutputPlugin

   Laravel Mix v6.0.11

✔ Compiled Successfully in 2688ms
┌────────────────────────────────────────┬──────────┐
│     File                               │ Size     │
├────────────────────────────────────────┼──────────┤
│      /build/app/images/icon-sprite.svg │ 1.5 KiB  │
│                  /build/app/js/main.js │ 25.5 KiB │
│        /build/app/js/raw/jquery.min.js │ 87.7 KiB │
│                 build/app/css/main.css │ 1 bytes  │
│                             mix.js.map │ 5.97 KiB │
└────────────────────────────────────────┴──────────┘
webpack compiled successfully in 2688 ms

All good so far.

Now, I'll upgrade sass from 1.42.1 to 1.43.2: npm ls diff output with previous npm ls:

├── argv@0.0.2
├── babel-eslint@10.1.0
├── browser-sync-webpack-plugin@2.3.0
├── browser-sync@2.26.14
├── dotenv@6.2.0
├── eslint-config-airbnb-base@14.2.1
├── eslint-plugin-import@2.22.1
├── eslint@7.19.0
├── instant.page@5.1.0
├── jquery@3.5.1
├── laravel-mix-merge-manifest@2.0.0
├── laravel-mix@6.0.11
├── npm-check@5.9.2
├── postcss@8.2.6
├── sass-loader@8.0.2
-├── sass@1.42.1
+├── sass@1.43.2
├── svg-sprite@1.5.0
└── tailwindcss@2.0.3

and the run command:

$ npm run prod:app-combine

> prod:app-combine
> mix --production -- --env --section=app

Browserslist: caniuse-lite is outdated. Please run:
npx browserslist@latest --update-db

Why you should do it regularly:
https://github.com/browserslist/browserslist#browsers-data-updating
ERROR in ./resources/assets/sass/main.scss
Module build failed (from ./node_modules/mini-css-extract-plugin/dist/loader.js):
ModuleBuildError: Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: Can't find stylesheet to import.
  ╷
4 │ @import "~sass/icon";
  │         ^^^^^^^^^^^^
  ╵
  /path/to/project/resources/assets/sass/main.scss 4:9  root stylesheet
    at processResult (/path/to/project/node_modules/webpack/lib/NormalModule.js:598:19)
    at /path/to/project/node_modules/webpack/lib/NormalModule.js:692:5
    at /path/to/project/node_modules/loader-runner/lib/LoaderRunner.js:399:11
    at /path/to/project/node_modules/loader-runner/lib/LoaderRunner.js:251:18
    at context.callback (/path/to/project/node_modules/loader-runner/lib/LoaderRunner.js:124:13)
    at /path/to/project/node_modules/sass-loader/dist/index.js:73:7
    at Function.call$2 (/path/to/project/node_modules/sass/sass.dart.js:96202:16)
    at render_closure1.call$2 (/path/to/project/node_modules/sass/sass.dart.js:82137:12)
    at _RootZone.runBinary$3$3 (/path/to/project/node_modules/sass/sass.dart.js:28242:18)
    at _FutureListener.handleError$1 (/path/to/project/node_modules/sass/sass.dart.js:26764:21)

1 ERROR in child compilations
webpack compiled with 2 errors in 637 ms

This shows that sass is somehow failing to find the file since 1.43.2.

Upon investigating I also noticed tags 1.43.0 and 1.43.1 were missing from the registry (https://www.npmjs.com/package/sass see versions)

Wondering what the reason could be I checked the diff of those versions on github: 1.43.0...1.43.1 => https://github.com/sass/dart-sass/compare/1.43.0...1.43.1 1.43.1...1.43.2 => https://github.com/sass/dart-sass/compare/1.43.1...1.43.2

Although dart isn't really my area of expertise, the commit content and commit messages in those diffs give me the impression that the file importer isn't working as expected.

Obviously I tried the latest sass version 1.43.4 as well to see if that fixes the import, but alas, it does not.

Sorry for the long comment. But given the confusion in the thread above I wanted to be as clear as possible :)

Hopefully this issue can be resolved soon as this bug has been (excuse the pun) bugging me for a couple of weeks now.

Levivb commented 2 years ago

is there more information needed? :)

stof commented 2 years ago

@nex3 between 1.42.1 and 1.43.2, the big change is the refactoring of the JS API to start working on the new one. Could it be that this refactoring broke the support of custom importers in the legacy JS API ? Is there any test covering them in the CI ? AFAICT, the tests in tests/legacy_node_api/ are not actually using the JS API itself as they are written in Dart, and the new JS tests in sass-spec are not covering importers.

nex3 commented 2 years ago

To be clear, what we need is one of two things:

The latter will take a lot more work to debug, but I can work with it. I can't work with descriptions of how it's failing that don't include code I can actually debug.

between 1.42.1 and 1.43.2, the big change is the refactoring of the JS API to start working on the new one. Could it be that this refactoring broke the support of custom importers in the legacy JS API ?

That probably has something to do with it, although figuring out exactly what will require debugging.

Is there any test covering them in the CI ? AFAICT, the tests in tests/legacy_node_api/ are not actually using the JS API itself as they are written in Dart, and the new JS tests in sass-spec are not covering importers.

Although the tests in tests/legacy_node_api are written in Dart, they are in fact executing the JS API through Dart's JS interop. Whatever's gone wrong here is subtle enough that it's not being caught by those tests.

paulfalgout commented 2 years ago

I had this issue and it took forever to track it down. But if I updated sass past 1.42.1 my webpack would fail and the sass-loader was never run during the failure.. determined by adding logging to the loader function..

As in the OP I too used the module resolve such that my imports looked like import 'sass/file.scss' but webpack was now resolving that as import 'sass'

The difference between 1.42.1 and later versions is the later versions include in the distributed package.json:

"exports":{"types":"./types/index.d.ts","default":"./sass.default.dart.js"}

If I dig down into my node_modules and remove this from the package.json 1.54.3 everything compiles nicely. But with these exports it fails.

The fix was to change my sass directory to scss such that what was previously import 'sass/file.scss'; became import scss/file.scss';. Though adding exports certainly introduced a breaking change IMO.

nex3 commented 2 years ago

Wow, what a mess.

It's worth noting that this is only a breaking change because of the specific details of the way Webpack's custom name resolution works. In vanilla Sass, npm packages have nothing to do with Sass name resolution. In vanilla Node, any load that begins with sass/ is considered the domain of the sass package and not of user code. That Webpack allowed you to break those invariants in the first place is arguably an issue with Webpack.

That said, Webpack is very widely-used and if we had known at the time that this would break this edge-case we might have tried to find another way around it. But at this point we can't change "exports" back to the way it was before because that would be a breaking change for anyone relying on the new behavior.

As a general rule, when you're working with Node-style imports (including Sass imports when you're using sass-loader), it's a good idea to avoid any URLs that begin with the name of an actual package. It's unfortunate that Webpack doesn't surface this error in a clearer way, though.