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

Cannot @import "bar.scss" when the extension is not .css (4.0.0 regression) #1164

Closed
andersk opened this issue Aug 12, 2020 · 17 comments
Closed

Comments

@andersk
Copy link

andersk commented Aug 12, 2020

css-loader ≥ 4.0.0 gives Error: Can't resolve 'bar.scss' when I try to @import "bar.scss". It works when the extension is .css but not when it’s .scss. I bisected this to commit edf5347 (#1099). The problem is triggered by the restrictions: [/\.css$/i] setting that was hard-coded here:

css-loader/src/index.js

Lines 50 to 56 in edf5347

const resolver = this.getResolve({
mainFields: ['css', 'style', 'main', '...'],
mainFiles: ['index', '...'],
extensions: ['.css'],
restrictions: [/\.css$/i],
conditionNames: ['style'],
});

  • Operating System: NixOS 20.09
  • Node Version: 14.7.0
  • NPM Version: 6.14.7
  • webpack Version: 4.44.1
  • css-loader Version: 4.2.1

Expected Behavior

With css-loader 3.6.0:

$ npx webpack
Hash: 18aeb4131231a0da692b
Version: webpack 4.44.1
Time: 122ms
Built at: 08/12/2020 4:28:49 PM
  Asset      Size  Chunks             Chunk Names
main.js  8.05 KiB    main  [emitted]  main
Entrypoint main = main.js
[./foo.scss] 396 bytes {main} [built]
[./node_modules/css-loader/dist/cjs.js!./bar.scss] 278 bytes {main} [built]
    + 1 hidden module
Done in 0.46s.

Actual Behavior

With css-loader ≥ 4.0.0:

$ npx webpack 
Hash: 8a377e383c6ebf971c58
Version: webpack 4.44.1
Time: 120ms
Built at: 08/12/2020 4:30:02 PM
  Asset      Size  Chunks             Chunk Names
main.js  6.36 KiB    main  [emitted]  main
Entrypoint main = main.js
[./foo.scss] 2.58 KiB {main} [built] [failed] [1 error]

ERROR in ./foo.scss
Module build failed (from ./node_modules/css-loader/dist/cjs.js):
Error: Can't resolve 'bar.scss' in '/tmp/b'
    at /tmp/b/node_modules/enhanced-resolve/lib/Resolver.js:209:21
    at /tmp/b/node_modules/enhanced-resolve/lib/Resolver.js:285:5
    at eval (eval at create (/tmp/b/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:13:1)
    at /tmp/b/node_modules/enhanced-resolve/lib/UnsafeCachePlugin.js:44:7
    at /tmp/b/node_modules/enhanced-resolve/lib/Resolver.js:285:5
    at eval (eval at create (/tmp/b/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:13:1)
    at /tmp/b/node_modules/enhanced-resolve/lib/Resolver.js:285:5
    at eval (eval at create (/tmp/b/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:25:1)
    at /tmp/b/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:67:43
    at /tmp/b/node_modules/enhanced-resolve/lib/Resolver.js:285:5
    at eval (eval at create (/tmp/b/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:26:1)
    at /tmp/b/node_modules/enhanced-resolve/lib/ModuleKindPlugin.js:30:40
    at /tmp/b/node_modules/enhanced-resolve/lib/Resolver.js:285:5
    at eval (eval at create (/tmp/b/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:13:1)
    at /tmp/b/node_modules/enhanced-resolve/lib/Resolver.js:285:5
    at eval (eval at create (/tmp/b/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:13:1)
    at /tmp/b/node_modules/enhanced-resolve/lib/forEachBail.js:30:14
    at /tmp/b/node_modules/enhanced-resolve/lib/Resolver.js:285:5
    at eval (eval at create (/tmp/b/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:13:1)
    at /tmp/b/node_modules/enhanced-resolve/lib/UnsafeCachePlugin.js:44:7
    at /tmp/b/node_modules/enhanced-resolve/lib/Resolver.js:285:5
    at eval (eval at create (/tmp/b/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:13:1)
    at /tmp/b/node_modules/enhanced-resolve/lib/Resolver.js:285:5
    at eval (eval at create (/tmp/b/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:25:1)
    at /tmp/b/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:67:43
    at /tmp/b/node_modules/enhanced-resolve/lib/Resolver.js:285:5
    at eval (eval at create (/tmp/b/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:14:1)
    at /tmp/b/node_modules/enhanced-resolve/lib/Resolver.js:285:5
    at eval (eval at create (/tmp/b/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:25:1)
    at /tmp/b/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:67:43

Code

// webpack.config.js
module.exports = {
  mode: "development",
  entry: "./foo.scss",
  module: {
    rules: [{ test: /\.s?css$/, use: "css-loader" }],
  },
};
/* foo.scss */
@import "bar.scss"
/* bar.scss */
html {
  background: red;
}

How Do We Reproduce?

Create the three files above and run npm i webpack webpack-cli css-loader; npx webpack.

andersk added a commit to andersk/css-loader that referenced this issue Aug 12, 2020
andersk added a commit to andersk/zulip that referenced this issue Aug 13, 2020
css-loader@^4 is also available, but we can’t use it yet because of
webpack-contrib/css-loader#1164.

Signed-off-by: Anders Kaseorg <[email protected]>
andersk added a commit to andersk/zulip that referenced this issue Aug 13, 2020
css-loader@^4 is also available, but we can’t use it yet because of
webpack-contrib/css-loader#1164.

Signed-off-by: Anders Kaseorg <[email protected]>
andersk added a commit to zulip/zulip that referenced this issue Aug 13, 2020
css-loader@^4 is also available, but we can’t use it yet because of
webpack-contrib/css-loader#1164.

Signed-off-by: Anders Kaseorg <[email protected]>
@catLotto

This comment has been minimized.

@alexander-akait
Copy link
Member

Yes, it is expected, I recommended refactor code and avoid it, without restrictions you can import JS file, one of the reasons why ES modules require extensions import test from '/scripts/test.mjs' is same, it is not safe, it is misleading, it is weird

@andersk
Copy link
Author

andersk commented Aug 13, 2020

I’m confused by this response…surely you aren’t recommending that all Webpack users abandon Sass, SCSS, Less, Stylus, PostCSS merely because “it is weird”? If not, how would you like this to be solved?

Note that my proposed patch does not allow @import "test" to implicitly import a file named test.js or test.mjs—it removes restrictions, not extensions. It allows you to explicitly @import "test.js", in which case test.js will be interpreted with CSS syntax (or whatever syntax is expected by importLoaders), which is exactly what would happen in a browser if a bundler were not in use.

MSurfer20 pushed a commit to MSurfer20/zulip that referenced this issue Aug 13, 2020
css-loader@^4 is also available, but we can’t use it yet because of
webpack-contrib/css-loader#1164.

Signed-off-by: Anders Kaseorg <[email protected]>
@alexander-akait
Copy link
Member

I’m confused by this response…surely you aren’t recommending that all Webpack users abandon Sass, SCSS, Less, Stylus, PostCSS merely because “it is weird”? If not, how would you like this to be solved?

Where I say it? I recommend do not use @import './file.scss' inside CSS file, it is weird, if you try to use it in browser, it was broken. Just migrate on SCSS fully, don't mix CSS and SCSS imports

Note that my proposed patch does not allow @import "test" to implicitly import a file named test.js or test.mjs—it removes restrictions, not extensions. It allows you to explicitly @import "test.js", in which case test.js will be interpreted with CSS syntax (or whatever syntax is expected by importLoaders), which is exactly what would happen in a browser if a bundler were not in use.

test doesn't mark sense here, when you write @import 'bootstrap', we should load CSS version, not SCSS, without restrictions we will do invalid resolution and you will open issue about it

@andersk
Copy link
Author

andersk commented Aug 14, 2020

Just migrate on SCSS fully, don't mix CSS and SCSS imports

I’m not mixing CSS and SCSS; this is about importing bar.scss from foo.scss. It’s true that I’ve omitted the configuration for SCSS above in the interest of minimizing the test case, but my actual configuration has

{
  loader: "css-loader",
  options: {
    importLoaders: 1,
    sourceMap: true,
  },
},
{
  loader: "postcss-loader",
  options: {
    sourceMap: true,
  },
},

with PostCSS using the postcss-scss parser.

/* postcss.config.js */
module.exports = ({file}) => ({
  parser: file.extname === ".scss" ? "postcss-scss" : false,
  plugins: {
    "postcss-nested": {},
    "postcss-extend-rule": {},
    "postcss-simple-vars": {},
    "postcss-calc": {},
    autoprefixer: {},
  },
});

Perhaps this is different from a typical sass-loader configuration in that sass-loader resolves @import itself and this PostCSS configuration does not. This is intentional in order to allow Webpack chunk splitting to work. Is that weird?

test doesn't mark sense here, when you write @import 'bootstrap', we should load CSS version, not SCSS, without restrictions we will do invalid resolution and you will open issue about it

In what way would @import 'bootstrap' get the SCSS version? The resolver configuration has mainFields: ['css', 'style', 'main', '...'], and bootstrap/package.json has "style": "dist/css/bootstrap.css" (and "sass": "scss/bootstrap.scss", but nothing in the configuration causes that to be used).

@alexander-akait
Copy link
Member

Please create reproducible test repo, restriction for css-loader do nothing for sass-loader, they used different resolvers and have different logic

@alexander-akait
Copy link
Member

You should not use postcss-scss parser for scsswhen you use postcss, because sass-loader return only valid CSS

@andersk
Copy link
Author

andersk commented Aug 17, 2020

sass-loader is not in my configuration. I don’t want sass-loader’s @import resolution, because it doesn’t work with Webpack chunk splitting. I use PostCSS plugins to enable needed SCSS features.

// webpack.config.js
module.exports = {
  mode: "development",
  entry: "./foo.scss",
  module: {
    rules: [
      {
        test: /\.scss$/,
        use: [
          "style-loader",
          { loader: "css-loader", options: { importLoaders: 1 } },
          "postcss-loader",
        ],
      },
    ],
  },
};
// postcss.config.js
module.exports = {
  parser: "postcss-scss",
  plugins: [
    // not important for this self-contained example
  ],
};
/* foo.scss */
@import "bar.scss";
/* bar.scss */
html {
  background: red;
}
npm i webpack webpack-cli style-loader postcss-loader postcss-scss css-loader
npx webpack

(If you want to look at the real-world code that ran into this, it’s https://github.com/zulip/zulip/blob/master/tools/webpack.config.ts.)

@alexander-akait
Copy link
Member

alexander-akait commented Aug 18, 2020

Your configuration mix CSS and SCSS @import's as I written above, it is limitation

@alexander-akait
Copy link
Member

alexander-akait commented Aug 18, 2020

Also using postcss-scss is not a safe for postcss-loader and plugins, you should avoid it, if you wan to use SCSS, please use sass-loader, postcss-scss was development for stylelint and other dev tools, not for bundling, it is just luck what you don't have problems with this

@andersk
Copy link
Author

andersk commented Aug 18, 2020

Your configuration mix CSS and SCSS @import's as I written above, it is limitation

I still don’t understand how an example with exactly one @import can possibly be mixing CSS and SCSS @imports. With css-loader 3 or with #1165, foo.scss and bar.scss are processed in exactly the same way.

If what you’re saying is that no @import should ever be allowed to make its way to css-loader unless it refers directly to a CSS file that requires no further processing, then what is supposed to be the point of importLoaders?

Also using postcss-scss is not a safe for postcss-loader and plugins, you should avoid it, if you wan to use SCSS, please use sass-loader, postcss-scss was development for stylelint and other dev tools, not for bundling, it is just luck what you don't have problems with this

I don’t see any documentation suggesting that postcss-scss is unsafe? I understand that it does not, by itself, provide all the features of Sass, but that doesn’t make it unsafe. I explained above why I don’t want to use sass-loader.

@alexander-akait
Copy link
Member

I still don’t understand how an example with exactly one @import can possibly be mixing CSS and SCSS @imports. With css-loader 3 or with #1165, foo.scss and bar.scss are processed in exactly the same way.

Because @import should load only CSS, I asked you to test it in the browser, but you ignored me

If what you’re saying is that no @import should ever be allowed to make its way to css-loader unless it refers directly to a CSS file that requires no further processing, then what is supposed to be the point of importLoaders?

No, you can still use postcss-loader and other loaders.

Please create reproducible test repo

Again ignore me.

parser: "postcss-scss", only parser file as SCSS it is not compile it, so I can't understand what variables are you talking about

I don’t see any documentation suggesting that postcss-scss is unsafe? I understand that it does not, by itself, provide all the features of Sass, but that doesn’t make it unsafe.

plugins: [
// not important for this self-contained example
],

Most of plugins for postcss designed for pure CSS, not for sass/scss, so I as written above postcss-scss should not be used here, it is for other dev tools, for example - stylelint uses it for parsing scss and linting, prettier for formatting.

I don't think we should document it here.

I explained above why I don’t want to use sass-loader.

You can create an own loader, if you have non standard approaches.

Sorry it is limitation, if you want to use sass/scss you need to use sass-loader.

@alexander-akait
Copy link
Member

You can't use typescript without typescript-loader, you can't use less without less-loader, so you can't use sass without sass-loader

@alexander-akait
Copy link
Member

And again:

/* postcss.config.js */
module.exports = ({file}) => ({
  parser: file.extname === ".scss" ? "postcss-scss" : false,
  plugins: {
    "postcss-nested": {},
    "postcss-extend-rule": {},
    "postcss-simple-vars": {},
    "postcss-calc": {},
    autoprefixer: {},
  },
});

From your config you try to imitate Sass-like syntax, so you should not use scss extension here, just use css, and you don't need postcss-scss here, it is create dangerous cases (I wish I could take the time and present them to you) and reduce build time, because parsing SCSS is slower

@andersk
Copy link
Author

andersk commented Aug 19, 2020

Because @import should load only CSS, I asked you to test it in the browser, but you ignored me

You never asked me to test it in the browser…all you said was “if you try to use it in browser, it was broken”. We both know how browsers work. Browsers do not care about file extensions. If the browser sees an @import, it will interpret that file as CSS.

That’s all I’m asking of css-loader. css-loader should not care about file extensions. If css-loader sees an @import, it should run that file through its importLoaders chain and interpret the result as CSS like it always has.

Please create reproducible test repo

Again ignore me.

Huh? I’ve given you two self-contained reproducible test cases (one, two) and one full real-world project. Is your complaint that I gave you the test files in the form of code blocks rather than in the form of a literal Git repository?

Most of plugins for postcss designed for pure CSS, not for sass/scss, so I as written above postcss-scss should not be used here, it is for other dev tools, for example - stylelint uses it for parsing scss and linting, prettier for formatting.

I don't think we should document it here.

This alleged restriction isn’t documented anywhere. The postcss-scss README suggests it as a fine way to allow // comments which are invalid in plain CSS.

But okay, if SCSS must be processed with sass-loader for some reason, here’s a reproducible test case with sass-loader, which does pass through @import to css-loader under certain circumstances such as media queries:

// webpack.config.js
module.exports = {
  mode: "development",
  entry: "./foo.scss",
  module: {
    rules: [
      {
        test: /\.scss$/,
        use: [
          "style-loader",
          { loader: "css-loader", options: { importLoaders: 1 } },
          "sass-loader",
        ],
      },
    ],
  },
};
// foo.scss
@import "bar.scss" (min-width: 992px);
// bar.scss
html {
  background: red;
}
npm i webpack webpack-cli style-loader css-loader sass sass-loader
npx webpack

(Same files as a Git repository.)

@alexander-akait
Copy link
Member

alexander-akait commented Aug 20, 2020

You never asked me to test it in the browser…all you said was “if you try to use it in browser, it was broken”. We both know how browsers work. Browsers do not care about file extensions. If the browser sees an @import, it will interpret that file as CSS.

No, it is worked on MIME type, otherwise it will be not safe

From chome:

For JS:

Refused to apply style from 'https://apis.google.com/js/client.js?onload=checkAuth' because its MIME type ('application/javascript') is not a supported stylesheet MIME type, and strict MIME checking is enabled.

For png:

Resource interpreted as Stylesheet but transferred with MIME type image/png

Yes, you can setup web server to send content-type: text/css, but scss still is not CSS and CSS parser will be failed with syntax.

Huh? I’ve given you two self-contained reproducible test cases (one, two) and one full real-world project. Is your complaint that I gave you the test files in the form of code blocks rather than in the form of a literal Git repository?

one - I write you - it is expected and it is limitation
two - I write you - why it is wrong and invalid, even more - it is unsafe and potential can break postcss plugins

This alleged restriction isn’t documented anywhere. The postcss-scss README suggests it as a fine way to allow // comments which are invalid in plain CSS.

Ok, but how does this relate to this problem?

But okay, if SCSS must be processed with sass-loader for some reason, here’s a reproducible test case with sass-loader, which does pass through @import to css-loader under certain circumstances such as media queries:

Just note - non JS never supported by webpack, it is feature for webpack@5, yes it worked in most of cases, but I warn you that this can be broken or lead to errors, this was not in the design for webpack@4

Because you mix CSS and SCSS again.

// foo.scss
@import "bar.scss" (min-width: 992px);

compiling to CSS:

@import "bar.scss" (min-width: 992px);

@alexander-akait
Copy link
Member

If you give me a link to the repository, I will give you my recommendations on how to properly organize it. You can take them into account if you wish. The main problem is that you mix CSS and scss imports, it's not quite right. Note that if this problem were popular there would already be many people here and 👍 . But most developers don't do this. So my recommendation is to stop doing that.

andersk added a commit to andersk/zulip that referenced this issue Sep 15, 2020
css-loader@4 broke @import statements referencing files with
extensions other than .css, unless those @import statements are
compiled away by another loader.  Upstream is more interested in
arguing that such @import statements are semantically incorrect than
applying the one line fix.

webpack-contrib/css-loader#1164

Signed-off-by: Anders Kaseorg <[email protected]>
timabbott pushed a commit to zulip/zulip that referenced this issue Sep 15, 2020
css-loader@4 broke @import statements referencing files with
extensions other than .css, unless those @import statements are
compiled away by another loader.  Upstream is more interested in
arguing that such @import statements are semantically incorrect than
applying the one line fix.

webpack-contrib/css-loader#1164

Signed-off-by: Anders Kaseorg <[email protected]>
Amitsinghyadav pushed a commit to Amitsinghyadav/zulip that referenced this issue Sep 20, 2020
css-loader@4 broke @import statements referencing files with
extensions other than .css, unless those @import statements are
compiled away by another loader.  Upstream is more interested in
arguing that such @import statements are semantically incorrect than
applying the one line fix.

webpack-contrib/css-loader#1164

Signed-off-by: Anders Kaseorg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants