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

fix: remove restrictions: [/\.css$/i] from resolver configuration #1165

Closed

Conversation

andersk
Copy link

@andersk andersk commented Aug 12, 2020

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Fixes #1164.

@jsf-clabot
Copy link

jsf-clabot commented Aug 12, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Aug 12, 2020

Codecov Report

Merging #1165 (779a022) into master (33e7879) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1165   +/-   ##
=======================================
  Coverage   99.36%   99.36%           
=======================================
  Files          10       10           
  Lines         627      627           
  Branches      197      197           
=======================================
  Hits          623      623           
  Misses          4        4           
Impacted Files Coverage Δ
src/index.js 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33e7879...779a022. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is break other resolutions, no you can import even JS file

@Cryszon
Copy link

Cryszon commented Mar 3, 2021

It is break other resolutions, no you can import even JS file

I'm using PostCSS, but cannot currently use .pcss extension with this loader. The reason for using .pcss instead of .css is for VS Code to detect the language automatically and display correct syntax highlighting.

I can understand not allowing .js or .scss imports (although I personally think it should be the developers responsibility to ensure correct import extensions), but do you have any suggestions on how to handle PostCSS files?

@alexander-akait
Copy link
Member

Why you need pcss? You can convert them using postcss-loader to CSS, this problem signals that you forgot to convert to CSS, pcss can contains invalid CSS syntax

@Cryszon
Copy link

Cryszon commented Mar 3, 2021

Here's a snippet of my webpack configuration for CSS:

      {
        test: /\.(p?css)$/,
        use: [
          { loader: MiniCssExtractPlugin.loader },
          { loader: "css-loader", options: { importLoaders: 1 } },
          "postcss-loader",
        ],
      },

And here's my postcss.config.js

const tailwindcss = require("tailwindcss");
const postcssPresetEnv = require("postcss-preset-env");

var config = ({ file, options, env }) => ({
  plugins: [
    tailwindcss,
    postcssPresetEnv({
      stage: 1,
    }),
  ],
});

module.exports = config;

Is there something I need to do to make PostCSS handle imports before css-loader?

Edit:
I just did a test with these two files to rule out syntax errors:
test.css

@import "test2.pcss";

h1 {
  color: red;
}

test2.pcss

h2 {
  color: blue;
}

And webpack gave me Error: Can't resolve 'test2.pcss'.

@alexander-akait
Copy link
Member

Here interesting problem, why it is hard to fix - some packages contain package.json with "main": "style.css" other "main": "main.js", so we need specify restrictions, otherwise we will get js file, if we remove it we will break compatibility with some packages, why it happen, because npm doesn't provide field to specify styles a long time ago, but now we have exports (condition names) and ideally package should migrate on it in future.

We are standing on a very shaky board - break compatibility with packages or allow to use non standard extensions in imports

@andersk
Copy link
Author

andersk commented Mar 3, 2021

This change will not cause anyone to get a .js file when they previously would have gotten a .css file (because it removes restrictions, not extensions). If a package foo specifies "main": "main.js", then @import "~foo" has never worked and there is no compatibility to break.

@alexander-akait
Copy link
Member

extensions is not affected on main, because it is different things

@alexander-akait
Copy link
Member

alexander-akait commented Mar 3, 2021

Ideally extensions should be [] here, because we allow to use @import 'index' (where index.css)

@alexander-akait
Copy link
Member

If a package foo specifies "main": "main.js", then @import "~foo" has never worked and there is no compatibility to break.

Some packages can contains "main": "style.css" and style: "style.scss", so when you use @import 'package'; we will get sass not css

@alexander-akait
Copy link
Member

But maybe we should start to say developers do not use main for CSS files and remove this restriction

@alexander-akait
Copy link
Member

Fixed #1272, hope we don't break something

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 this pull request may close these issues.

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