fix(css): no emit assets in html style tag (use class) (fix #5968) #6048

poyoho · 2021-12-10T02:02:22Z

Description

fix: #5968

Additional context

had the better way to use async method in the traverseHtml?
can closures be avoided createHTMLCSSCompiler ?

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.

packages/vite/src/node/plugins/css.ts

packages/vite/src/node/plugins/html.ts

poyoho · 2021-12-17T08:23:51Z

oh, how to fix the test error

bluwy

I think the code could use some comments to describe each part's goal, it took me a while to understand the flow. I have one concern below, but otherwise this looks good to me. Pretty neat implementation too! I thought it would be longer.

I'd also prefer if another maintainer more experienced with this could take a look, this is my first time understanding this file 😅

PS: Feel free to add more comments where you see fit.

packages/vite/src/node/plugins/html.ts

bluwy · 2021-12-17T12:41:04Z

packages/vite/src/node/plugins/html.ts

+            js += `\nimport "${id}?html-proxy&index=${inlineModuleIndex}.css"`
+            if (classPropsNode) {
+              const classPropValue = classPropsNode.value!
+              s.remove(inlineStyle.loc.start.offset, inlineStyle.loc.end.offset)


Do we need to remove the inline style if classPropsNode is false too?

false will overwrite the style prop to the class.

Ah yes. Thanks for the explanation

patak-dev · 2021-12-17T12:47:54Z

I'm also checking, LGTM too. We are going to try to discuss it in the next team meeting
I think we should add it to the 2.8 milestone (beta should be triggered soon), and maybe we should rename vite:html-inline-script-style-proxy to vite:html-inline-proxy

poyoho · 2021-12-17T12:50:55Z

ok, I will comment it

Co-authored-by: Bjorn Lu <[email protected]>

packages/vite/src/node/plugins/html.ts

packages/vite/src/node/plugins/index.ts

Co-authored-by: Shinigami <[email protected]>

bluwy

Looking good!

ydcjeff · 2021-12-20T05:08:52Z

Does it PR has any difference with #6181?

poyoho · 2021-12-20T05:10:38Z

yes, but I don't know how to mentioned pull request😅

poyoho · 2021-12-20T15:13:46Z

Please review at the mentioned PR @patak-dev @Shinigami92 @bluwy.
I think use class to resolve inline style is not a best way.That will change CSS priority.

bluwy · 2021-12-23T07:54:08Z

@poyoho Should we close this PR if #6181 supersedes this? It's a bit confusing to have two same PR names.

poyoho · 2021-12-23T07:55:57Z

I'm not sure which way to take in the end. I'll change PR name?

poyoho added 8 commits December 9, 2021 12:11

test: add url import assets in style tag / inline style tests

ba5ac17

fix: compile style / inline style in index.html

93ef3f3

test: @import test

4736fe8

chore: test unit

5004fc3

feat: ignore repeat to create resolve

6bc8dfa

chore: format

26dc100

Merge branch 'main' into fix/assets-in-style

ab61747

fix: never used import

1005c06

poyoho closed this Dec 10, 2021

fix: fix resolve importer id

827e94d

poyoho reopened this Dec 10, 2021

Niputi added the feat: css label Dec 10, 2021

Shinigami92 suggested changes Dec 11, 2021

View reviewed changes

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved

feat: define createHTMLCSSCompiler return type

f1ef753

Shinigami92 previously approved these changes Dec 11, 2021

View reviewed changes

bluwy reviewed Dec 11, 2021

View reviewed changes

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved

feat: find the css plugin instance re-using transform method

610d9f8

poyoho dismissed Shinigami92’s stale review via 610d9f8 December 12, 2021 00:09

poyoho changed the title ~~fix(css): assets in style no emit~~ fix(css): assets in style no emit (fix: #5968) Dec 12, 2021

poyoho changed the title ~~fix(css): assets in style no emit (fix: #5968)~~ fix(css): assets in style no emit (fix #5968) Dec 12, 2021

Shinigami92 previously approved these changes Dec 12, 2021

View reviewed changes

poyoho changed the title ~~fix(css): assets in style no emit (fix #5968)~~ fix(css): no emit assets in html style tag (fix #5968) Dec 12, 2021

poyoho requested a review from bluwy December 12, 2021 00:38

bluwy reviewed Dec 16, 2021

View reviewed changes

packages/vite/src/node/plugins/html.ts Outdated Show resolved Hide resolved

refactor: import proxy handle css tag in html

8cf63a3

poyoho dismissed Shinigami92’s stale review via 8cf63a3 December 17, 2021 07:56

poyoho added 2 commits December 17, 2021 16:06

Merge branch 'main' into fix/assets-in-style

380e0f5

chore: format

676b01b

poyoho requested a review from bluwy December 17, 2021 08:14

Shinigami92 previously approved these changes Dec 17, 2021

View reviewed changes

bluwy reviewed Dec 17, 2021

View reviewed changes

poyoho dismissed Shinigami92’s stale review via 013071f December 17, 2021 13:17

poyoho and others added 4 commits December 17, 2021 21:17

feat commit for style tag in html emit asset pipline

013071f

Co-authored-by: Bjorn Lu <[email protected]>

feat: commit for style tag in html emit asset pipline

c16aec7

Co-authored-by: Bjorn Lu <[email protected]>

Merge branch 'vitejs:main' into fix/assets-in-style

e62234d

feat: comment

6e02f9b

Shinigami92 reviewed Dec 18, 2021

View reviewed changes

poyoho and others added 5 commits December 19, 2021 07:51

fix: html-inline-script-style-proxy -> html-inline-proxy

b41e001

Co-authored-by: Shinigami <[email protected]>

fix: html-inline-script-style-proxy -> html-inline-proxy

d533d2a

Co-authored-by: Shinigami <[email protected]>

fix: html-inline-script-style-proxy -> html-inline-proxy

b2c6756

Co-authored-by: Shinigami <[email protected]>

fix: html-inline-script-style-proxy -> html-inline-proxy

a19bdda

Co-authored-by: Shinigami <[email protected]>

chore: format

1a0400c

patak-dev added this to the 2.8 milestone Dec 19, 2021

bluwy previously approved these changes Dec 19, 2021

View reviewed changes

Shinigami92 previously approved these changes Dec 19, 2021

View reviewed changes

Merge branch 'main' into fix/assets-in-style

68d90e2

poyoho dismissed stale reviews from Shinigami92 and bluwy via 68d90e2 December 19, 2021 23:55

poyoho changed the title ~~fix(css): no emit assets in html style tag (fix #5968)~~ fix(css): no emit assets in html style tag (use class) (fix #5968) Dec 23, 2021

poyoho closed this Dec 23, 2021

patak-dev removed this from the 2.8 milestone Dec 26, 2021

poyoho mentioned this pull request Dec 30, 2021

fix(css): no emit assets in html style tag (use map) (fix #5968) #6181

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(css): no emit assets in html style tag (use class) (fix #5968) #6048

fix(css): no emit assets in html style tag (use class) (fix #5968) #6048

poyoho commented Dec 10, 2021 •

edited

Loading

poyoho commented Dec 17, 2021

bluwy left a comment •

edited

Loading

bluwy Dec 17, 2021

poyoho Dec 17, 2021 •

edited

Loading

bluwy Dec 19, 2021

patak-dev commented Dec 17, 2021

poyoho commented Dec 17, 2021

bluwy left a comment

ydcjeff commented Dec 20, 2021

poyoho commented Dec 20, 2021

poyoho commented Dec 20, 2021 •

edited

Loading

bluwy commented Dec 23, 2021

poyoho commented Dec 23, 2021 •

edited

Loading

fix(css): no emit assets in html style tag (use class) (fix #5968) #6048

fix(css): no emit assets in html style tag (use class) (fix #5968) #6048

Conversation

poyoho commented Dec 10, 2021 • edited Loading

Description

Additional context

What is the purpose of this pull request?

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

poyoho commented Dec 17, 2021

bluwy left a comment • edited Loading

Choose a reason for hiding this comment

bluwy Dec 17, 2021

Choose a reason for hiding this comment

poyoho Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

bluwy Dec 19, 2021

Choose a reason for hiding this comment

patak-dev commented Dec 17, 2021

poyoho commented Dec 17, 2021

bluwy left a comment

Choose a reason for hiding this comment

ydcjeff commented Dec 20, 2021

poyoho commented Dec 20, 2021

poyoho commented Dec 20, 2021 • edited Loading

bluwy commented Dec 23, 2021

poyoho commented Dec 23, 2021 • edited Loading

poyoho commented Dec 10, 2021 •

edited

Loading

bluwy left a comment •

edited

Loading

poyoho Dec 17, 2021 •

edited

Loading

poyoho commented Dec 20, 2021 •

edited

Loading

poyoho commented Dec 23, 2021 •

edited

Loading