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: style use string instead of js import #7786

Merged
merged 3 commits into from Apr 22, 2022
Merged

Conversation

poyoho
Copy link
Member

@poyoho poyoho commented Apr 18, 2022

Description

as @IanVS said

I have inline styles in my head, but they're being extracted into a script. so I have unstyled content that flashes when the page is loading.

so make style tag use string instead of js import

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@patak-dev
Copy link
Member

@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.

@IanVS
Copy link
Contributor

IanVS commented Apr 18, 2022

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?

@IanVS
Copy link
Contributor

IanVS commented Apr 18, 2022

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.

@meduzen
Copy link

meduzen commented Apr 18, 2022

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’m not sure how I can test. I grabbed the branch (fix/css-hmr from @poyoho’s fork) and tried to follow the contribution steps, but it failed at pnpm run build (using Node 14.17.3, macOS 11.6.5, Intel CPU).

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.

@poyoho
Copy link
Member Author

poyoho commented Apr 18, 2022

you can do follow

  1. fork this PR in your local vite repo.
  2. Build local vite
  3. link the vite to your project. https://github.com/vitejs/vite/blob/main/CONTRIBUTING.md#testing-vite-against-external-packages
  4. and build your project

And run your test case.

@meduzen
Copy link

meduzen commented Apr 18, 2022

2. Build it

“it” being building Vite, or building my project using the linked Vite repo?

Because as said in my previous comment, running pnpm run build from the Vite PR fails.

@poyoho
Copy link
Member Author

poyoho commented Apr 18, 2022

edited.

Copy link
Member

@bluwy bluwy left a 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.

@patak-dev patak-dev merged commit ba43c29 into vitejs:main Apr 22, 2022
@poyoho poyoho deleted the fix/css-hmr branch April 22, 2022 06:34
IanVS added a commit to storybookjs/builder-vite that referenced this pull request Apr 22, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants