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

@import with media query produces incorrect output when the imported file is a entry point #1503

Closed
silverwind opened this issue Mar 15, 2023 · 27 comments · Fixed by #1504 or webpack-contrib/mini-css-extract-plugin#1021

Comments

@silverwind
Copy link

silverwind commented Mar 15, 2023

Feature Proposal

Support @import list-of-media-queries syntax. Currently the plugin ignores and strips the media query from the output, making runtime-conditional import impossible without additional plugins.

Feature Use Case

@import "./dark.css" (prefers-color-scheme: dark);

Currently the plugin outputs the content of dark.css file without any media query condition, so the CSS is applied unconditionally at runtime:

/* content of dark.css */

A better output would be:

@media (prefers-color-scheme: dark) {
  /* content of dark.css */
}
@silverwind
Copy link
Author

silverwind commented Mar 15, 2023

Seems I found a solution by adding postcss-loader with thepostcss-import plugin. I still wonder if this feature would be considered for inclusion in css-loader as I feel like this should be possible without postcss baggage.

silverwind added a commit to silverwind/gitea that referenced this issue Mar 15, 2023
Fix regression from go-gitea#23481.

The conditional CSS import was being stripped away by webpack's
`css-loader`, resulting in the dark theme always loading. Unfortunately,
we have to re-introduce postcss to the CSS pipeline to fix this and I
loaded only the minimal plugins to make it work.

Related: webpack-contrib/css-loader#1503
techknowlogick pushed a commit to go-gitea/gitea that referenced this issue Mar 15, 2023
Fix regression from #23481.

The conditional on the CSS import was being stripped away by webpack's
`css-loader`, resulting in the dark theme always loading. The old syntax
with `@import` nested inside `@media` also did not work as `css-loader`
(rightfully) ignores such non-standard `@import` syntax that was
previously supported by Less.

Unfortunately, we have to re-introduce postcss to the CSS pipeline to
fix this and I loaded only the minimal plugins to make it work.

There is one variant of the fix that does work without postcss, which is
to exclude the file from transpilation but I did not consider it as it
would have meant the `@import` was being done without a version suffix
in the URL, which would have caused cache issue.

Related: webpack-contrib/css-loader#1503

---------

Co-authored-by: John Olheiser <john.olheiser@gmail.com>
@alexander-akait
Copy link
Member

Are you sure you are using the latest version? It was implement a long time ago, we supports @layer too

#1504

exports[`"import" option should work and output media: result 1`] = `
"@media (prefers-color-scheme: dark) {a {
  color: white;
}
}a {
  color: black;
}
"
`;

@alexander-akait
Copy link
Member

Anyway feel free to feedback, and check style-loader and mini-css-extract-plugin version too

@silverwind
Copy link
Author

silverwind commented Mar 15, 2023

This was the setup where I observed it:

https://github.com/go-gitea/gitea/blob/202803fc69d21763a06d8d0f5a4c46509c18f6f1/webpack.config.js#L138-L153

No style-loader, mini-css-extract-plugin very slightly out of date by one patch release. The import was not filtered. I guess I have to retry with a clean setup, after seeing your tests.

@alexander-akait
Copy link
Member

As you can see we have another result... I can provie history when it was implemented and can provide usage in other repos

@silverwind
Copy link
Author

Sure thanks, that's more than I expected from this issue. Now it's on me to debug further :)

@alexander-akait
Copy link
Member

alexander-akait commented Mar 15, 2023

Hm, you have vue-loader, and they are registering own style-loader (which I think doesn't support it), where do you use this styles?

@alexander-akait
Copy link
Member

alexander-akait commented Mar 15, 2023

Can you try to create simple CSS file and do it, like I done in tests

@alexander-akait
Copy link
Member

I really want to help, but as you can see we don't have problem inside css-loader

@silverwind
Copy link
Author

Just this plain CSS file, so .vue loader is not involved:

https://github.com/go-gitea/gitea/blob/main/web_src/css/themes/theme-auto.css

@silverwind
Copy link
Author

Yes, I will try to make a minimal webpack repro.

@alexander-akait
Copy link
Member

alexander-akait commented Mar 15, 2023

Just this plain CSS file, so .vue loader is not involved:

https://github.com/go-gitea/gitea/blob/main/web_src/css/themes/theme-auto.css

Vue plugin can register a loader on css under the hood (I don't know how it is now, but before it was), so you can think that style-loader used, but no

@silverwind
Copy link
Author

Okay, let me see what happens if I remove vue-loader.

@alexander-akait
Copy link
Member

Yeah, let's try, it is the first place where we potentially can have a probem

@silverwind
Copy link
Author

Unfortunately, no amount of plugin removal has any effect, the imported CSS is inlined without any @media wrapping.

image

So I think I will make a fresh repo next.

@alexander-akait
Copy link
Member

Yeah, I will keep open, feel free to ping me

@silverwind
Copy link
Author

silverwind commented Mar 15, 2023

Just fyi, the minimal repro so far does not reproduce it, so it's definitely something else in that config interfering.

https://github.com/silverwind/media-url

It correctly outputs

@media (prefers-color-scheme: dark) {
body { background: black }

}

Will check further what else in the full config could be a cause.

@alexander-akait
Copy link
Member

To be honest, I don't see anything criminal in the configuration...

@silverwind
Copy link
Author

silverwind commented Mar 15, 2023

Added the original CSS files to https://github.com/silverwind/media-url, now it reproduces and the @media is gone in output. It seems it has something to do with the file that is being imported already being present in another entrypoint.

$ git clone https://github.com/silverwind/media-url && cd media-url
$ npm install
$ npx webpack
$ head -5 dist/theme-auto.css
/*!***************************************************************************************************!*\
  !*** css ./node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[0].use[1]!./css/chroma/base.css ***!
  \***************************************************************************************************/
.chroma {
  background-color: var(--color-code-bg);

Trying to reduce this further now...

@silverwind
Copy link
Author

Yep, as soon as I remove the separate entry point for the file that is being imported, the @media is emitted correctly.

@alexander-akait
Copy link
Member

I see...give me time to think how I can fix it and there is a problem (I think in MiniCssExtractPlugin.loader, but need check)

@silverwind
Copy link
Author

https://github.com/silverwind/media-url is minimal again.

With the two entry points, it fails to emit @media in index:

    dark: fileURLToPath(new URL('dark.css', import.meta.url)),
    index: fileURLToPath(new URL('index.css', import.meta.url)),

If I remove the dark entry point, the @media in index is back again.

@silverwind silverwind changed the title Support @import url list-of-media-queries @import with media query produces incorrect output when the imported file is a entry point Mar 16, 2023
silverwind added a commit to silverwind/gitea that referenced this issue Mar 16, 2023
Fix regression from go-gitea#23481.

The conditional on the CSS import was being stripped away by webpack's
`css-loader`, resulting in the dark theme always loading. The old syntax
with `@import` nested inside `@media` also did not work as `css-loader`
(rightfully) ignores such non-standard `@import` syntax that was
previously supported by Less.

Unfortunately, we have to re-introduce postcss to the CSS pipeline to
fix this and I loaded only the minimal plugins to make it work.

There is one variant of the fix that does work without postcss, which is
to exclude the file from transpilation but I did not consider it as it
would have meant the `@import` was being done without a version suffix
in the URL, which would have caused cache issue.

Related: webpack-contrib/css-loader#1503

---------

Co-authored-by: John Olheiser <john.olheiser@gmail.com>
@alexander-akait
Copy link
Member

alexander-akait commented Mar 16, 2023

Yes, I was right, the problem is in the mini-css-extract-plugin, a fix is simple:

identifier() {
  return `css|${this._identifier}|${this._identifierIndex}|${this.media}|${this.supports}|${this.layer}`;
}

https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/src/index.js#L168

I am very tired today because I moved to a new country and will send a PR and will do release tomorrow, but if you want to send a PR yourself, feel free you have the fix and the test case, in this case I will do just release 😄

@silverwind
Copy link
Author

No hurry, we have a workaround via postcss until then. But I'll be very happy to rip it out again after this fix is in 😄.

@alexander-akait
Copy link
Member

@alexander-akait
Copy link
Member

Should work 👍

@silverwind
Copy link
Author

Confirmed, thanks for the quick fix! 🎉

techknowlogick added a commit to go-gitea/gitea that referenced this issue Mar 16, 2023
Follow-up and proper fix for
#23504

Update to
[mini-css-extract-plugin@2.7.4](https://github.com/webpack-contrib/mini-css-extract-plugin/releases/tag/v2.7.4)
which fixes our specific issue described in
webpack-contrib/css-loader#1503 and which
allows us to again drop the postcss dependency.

Backport of this is not necessary as I have included it in
#23508.

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants