feat: deprecated default export of css imports#10762
feat: deprecated default export of css imports#10762manucorporat wants to merge 2 commits intovitejs:mainfrom
Conversation
|
I think this is not a bad idea. To avoid disruption, if we go this way, maybe we should merge this PR in Vite 4, and then remove support next year with a hard error in Vite 5. @benmccann @matthewp @danielroe interested in your thoughts about this one. |
|
The |
|
@benmccann indeed! just to clarify this PR is about reducing the duplicate loading of CSS, if developer are already doing side effect import of CSS, it would still work! but if they wish to consume the CSS as a JS string, then they need to use @patak-dev fan to avoid disruption as much as possible, current PR will generate something like this in users code if merged for Vite 4: but it will still work, purely a TS advisory Looked in the while, and seems like 90% of the usage of CSS is already side-effect only, so hopefully affects very little users |
|
Agreed. I don't think this move (deprecating in this PR and then removing support in Vite 5) poses any issues from a Nuxt perspective. As I may have mentioned before, we are currently using |
|
We discussed the proposal in today's team meeting and we think this is a good idea, we should avoid potential unintended duplicated CSS here. @manucorporat we would want to merge this for Vite 4, and then remove support in Vite 5 to avoid disruption. We think that the typescript error is a great idea, but we should also add a runtime log when it is used. Let us know if you want to tackle this one, or if not we can add the log in this PR ourselves. I think it is also a good idea to use this PR to properly document @danielroe @sapphi-red mentioned we may be able to remove the |
|
@manucorporat can you allow maintainers to push up to the fork? I've added the deprecation messages locally |
|
mmm how do i do that? |
|
@manucorporat check for docs here. I think if next time you do this when you create a PR it should remember it moving forward
|
|
I'm not sure if the permissions are different for org forks, but if it's not possible I could open a new PR if you dont mind. |
|
It is breaking some styling when injected into shadow dom of a custom element... |
|
@GabrielHangor please create an issue with a minimal reproduction if you think we could improve this so we can track it, your comment is going to be lost here in the PR |
This commit follows the [patch for Turnstile](#7854) and performs a similar operation for the Storybook build, which failed after the latest `npm audit` and `npm update` passes. [This patch to Vite](vitejs/vite#10762) fixes a problem with the Vite build in that Vite could not resolve if a CSS import was strictly at the module level or if it was necessary to include the imported CSS at the document level. The fix is to hack a query, `?inline`, to the end of the import string, to indicate that it's a module-only import. The Storybook for Web Components build recommended by the Open Webcomponent Consortium is a Storybook-Vite implementation. The latest update fully deprecated undecorated CSS imports, and Storybook broke, unable to reconcile the CSS imports. This patch inlines the inlining of the CSS automatically for Storybook by using the Rollup `modify()` plug-in which performs string substitutions on the source code before it's presented to the compiler and bundler; it recognizes the strings that require inlining, those that match the regex: ``` JavaScript /^(import \w+ from .*\.css)";/ ``` ... and replaces them with a version ending in `.css?inline`. Because the actual recognizer inside `modify()` recognizes strings and not regular expressions, a script to build the strings has been added to the `scripts` folder. Just like locales, you will have to re-run and re-build `build-storybook-import-maps` script if you add a new CSS file to the source tree.
This commit follows the [patch for Turnstile](#7854) and performs a similar operation for the Storybook build, which failed after the latest `npm audit` and `npm update` passes. [This patch to Vite](vitejs/vite#10762) fixes a problem with the Vite build in that Vite could not resolve if a CSS import was strictly at the module level or if it was necessary to include the imported CSS at the document level. The fix is to hack a query, `?inline`, to the end of the import string, to indicate that it's a module-only import. The Storybook for Web Components build recommended by the Open Webcomponent Consortium is a Storybook-Vite implementation. The latest update fully deprecated undecorated CSS imports, and Storybook broke, unable to reconcile the CSS imports. This patch inlines the inlining of the CSS automatically for Storybook by using the Rollup `modify()` plug-in which performs string substitutions on the source code before it's presented to the compiler and bundler; it recognizes the strings that require inlining, those that match the regex: ``` JavaScript /^(import \w+ from .*\.css)";/ ``` ... and replaces them with a version ending in `.css?inline`. Because the actual recognizer inside `modify()` recognizes strings and not regular expressions, a script to build the strings has been added to the `scripts` folder. Just like locales, you will have to re-run and re-build `build-storybook-import-maps` script if you add a new CSS file to the source tree.
* web: fix storybookbuild build after npm update This commit follows the [patch for Turnstile](#7854) and performs a similar operation for the Storybook build, which failed after the latest `npm audit` and `npm update` passes. [This patch to Vite](vitejs/vite#10762) fixes a problem with the Vite build in that Vite could not resolve if a CSS import was strictly at the module level or if it was necessary to include the imported CSS at the document level. The fix is to hack a query, `?inline`, to the end of the import string, to indicate that it's a module-only import. The Storybook for Web Components build recommended by the Open Webcomponent Consortium is a Storybook-Vite implementation. The latest update fully deprecated undecorated CSS imports, and Storybook broke, unable to reconcile the CSS imports. This patch inlines the inlining of the CSS automatically for Storybook by using the Rollup `modify()` plug-in which performs string substitutions on the source code before it's presented to the compiler and bundler; it recognizes the strings that require inlining, those that match the regex: ``` JavaScript /^(import \w+ from .*\.css)";/ ``` ... and replaces them with a version ending in `.css?inline`. Because the actual recognizer inside `modify()` recognizes strings and not regular expressions, a script to build the strings has been added to the `scripts` folder. Just like locales, you will have to re-run and re-build `build-storybook-import-maps` script if you add a new CSS file to the source tree. * web: prettier had opinions * web: apply eslint + sonarjs check to the scripts folder. * Google recaptcha (aka Turnstile) doesn't understand the "invisible" setting; that's purely an HCaptcha thing. * web: removing the typecast means I no longer need the type. * web: prettier is still having opinions, dammit.
Description
Importing a css like:
Will fundamentally cause a double loading of CSS, since vite will emit a .css file, but it;s likely that the CSS will also be used by the application code, being most likely used by the framework runtime to inject the style.
This is actually hard to detect since, the API does not indicate there is a side effect anywhere. Today's solution is to add
?inline, but it's a lie, since the original API is also inlined.?inlineactually means (ONLY inline, no side effect). Imo, this is wrong.This PR will not cause any breaking changes, but deprecated the usage of:
giving a recomendation to use:
Additional context
#10235
QwikDev/qwik#1932
QwikDev/qwik#749 (comment)
QwikDev/qwik#1959
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).