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(css): no emit assets in html style tag (use map) (fix #5968) #6181

Closed
wants to merge 39 commits into from
Closed

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

wants to merge 39 commits into from

Conversation

poyoho
Copy link
Member

@poyoho poyoho commented Dec 19, 2021

Description

fix #5968

Additional context

I think it is better way to resolve inline style.

  • use js import to use css plugin transform inline style. and relace it with a flag, like assets url.
  • css-post plugin record transform result.use result replace flag when generateBundle.

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.

poyoho and others added 27 commits December 9, 2021 12:11
@poyoho poyoho marked this pull request as ready for review December 19, 2021 15:43
@poyoho 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 map) (fix #5968) Dec 23, 2021
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.

IIRC, this PR solves the issue in the build output so that inline styles that are converted into CSS (with a custom class name) are not generated as a separate file, which is denoted by &inline. This is a wild ride 😄

I've made a few comments below, and we'll probably want to add a test for CSS specificity as well.

I have two concerns though:

  1. &inline or ?inline is already a valid suffix for CSS imports. I've only discovered this recently and it seems to work like ?raw, though I'm still not familiar how it works. Is it intentional that you choose the &inline query as well? We might want to rename it in case it clashes somehow.
  2. Probably worth a discussion too if we want to support url() in style=""? It seems to complicate the flow a lot, though I really appreciate that you've already found a solution for it.

@poyoho poyoho requested a review from bluwy December 23, 2021 11:24
bluwy
bluwy previously approved these changes Dec 23, 2021
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.

Overall looks good to me! I think there's still merit to discuss the below, but it's an approval for me.

Probably worth a discussion too if we want to support url() in style=""? It seems to complicate the flow a lot


Also if #6048 does have css specificity issue that this PR fixes, we should close that one in favour of this.

@poyoho
Copy link
Member Author

poyoho commented Dec 23, 2021

yes, we can find the css plugin transfrom it or export transform css method.
now the method refers to the asset plugin, to tell the truth, it's really complicated.😅

@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label Dec 26, 2021
Shinigami92
Shinigami92 previously approved these changes Dec 26, 2021
@Shinigami92
Copy link
Member

@poyoho I think the line got to long 😱
You may need to run format

@poyoho
Copy link
Member Author

poyoho commented Dec 26, 2021

Why is it not automatically formatted🤣

@Shinigami92
Copy link
Member

Why is it not automatically formattedrofl

I think it has something todo with lint-stage or maybe husky that will come in another PR
And even if, there are ways to bypass it 🙁
So we added this check that cannot be bypassed 🙂

@poyoho
Copy link
Member Author

poyoho commented Dec 26, 2021

lint-staged is good for me 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assets in <style> tags are not processed by vite build
5 participants