-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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: style use string instead of js import #7786
Conversation
@IanVS would you help test this one? I think we should merge this one in the 2.9 series, even if the change isnt trivial, I think not being able to have css inlined is quite surprising. |
I am not able to test this right now, but maybe someone who commented on #6737 can. I think this PR fixes that issue, right? |
I briefly tested this in the vite builder, and it seems to work as intended. But, I didn't have time to review or check thoroughly. But, I agree that this is a 2.9 regression and should be released as a patch. |
I’m not sure how I can test. I grabbed the branch ( stack trace❯ pnpm run build
> vite-monorepo@ build /Users/my-username/Code/vite
> run-s build-vite build-plugin-vue build-plugin-react
> vite-monorepo@ build-vite /Users/my-username/Code/vite
> cd packages/vite && npm run build
> vite@2.9.5 build
> rimraf dist && npm run lint && run-s build-bundle build-types
> vite@2.9.5 lint
> eslint --ext .ts src/**
> vite@2.9.5 build-bundle
> rollup -c
/Users/my-username/Code/vite/packages/vite/src/client/env.ts → dist/client/env.mjs...
created dist/client/env.mjs in 1.6s
/Users/my-username/Code/vite/packages/vite/src/client/client.ts → dist/client/client.mjs...
created dist/client/client.mjs in 1.3s
/Users/my-username/Code/vite/packages/vite/src/node/index.ts, /Users/my-username/Code/vite/packages/vite/src/node/cli.ts → dist...
shimmed: plugins/terser.ts
shimmed: cac/dist/index.mjs
shimmed: fsevents-handler.js
shimmed: process-content.js
shimmed: lilconfig/dist/index.js
created dist in 18.4s
/Users/my-username/Code/vite/node_modules/.pnpm/terser@5.12.1/node_modules/terser/dist/bundle.min.js → dist...
created dist in 1.1s
> vite@2.9.5 build-types
> run-s build-temp-types patch-types roll-types
> vite@2.9.5 build-temp-types
> tsc --emitDeclarationOnly --outDir temp/node -p src/node
> vite@2.9.5 patch-types
> ts-node scripts/patchTypes.ts
patched types/* imports
> vite@2.9.5 roll-types
> api-extractor run && rimraf temp
api-extractor 7.21.2 - https://api-extractor.com/
Using configuration from ./api-extractor.json
Analysis will use the bundled TypeScript version 4.5.4
ERROR: Error reading "/Users/my-username/Code/vite/packages/vite/types/package.json":
The required field "name" was not found
ERROR: "roll-types" exited with 1.
ERROR: "build-types" exited with 1.
ELIFECYCLE Command failed with exit code 1.
ERROR: "build-vite" exited with 1.
ELIFECYCLE Command failed with exit code 1. Is there another way I can test it? Otherwise, anyone can grab the project mentioned in this comment to test it. |
you can do follow
And run your test case. |
“it” being building Vite, or building my project using the linked Vite repo? Because as said in my previous comment, running |
edited. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense that the end-user has a choice to inline or not inline (via css import). I think it's generally safe to merge too.
Fixes #343 This replaces the blunt hammer of ```css body > * { display: none !important; } ``` with something a bit more nuanced, taken from the storybook default head styles. This is only necessary until vitejs/vite#7786 is released, then we can remove the workaround and ask users to update vite in order to avoid this flash.
Description
as @IanVS said
so make style tag use string instead of js import
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).