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: handle url imports with semicolon (fix #7717) #7718

Merged
merged 4 commits into from Apr 13, 2022

Conversation

hugoattal
Copy link
Contributor

fix #7717

Description

Handle CSS Url imports, example:

@import url('https://fonts.googleapis.com/css2?family=Poppins:wght@200;400;600;800&family=Rubik:wght@400&display=swap');

cf
https://developer.mozilla.org/en-US/docs/Web/CSS/@import
https://developer.mozilla.org/en-US/docs/Web/CSS/url

Additional context

I handled only url since I'm not sure any other thing can cause this bug...


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.

@hugoattal hugoattal changed the title fix: handle url imports with semicolon (close #7717) fix: handle url imports with semicolon (fix #7717) Apr 13, 2022
@patak-dev
Copy link
Member

Thanks for the PR @hugoattal!

I'm testing with a few more URLs from the linked MDN page:

// Works!
@import url("fineprint.css") print;
@import url("bluish.css") speech;
@import "common.css" screen;
@import url('landscape.css') screen and (orientation:landscape);
@import url(https://example.com/images/myImg.jpg);
@import url(myFont.woff);
@import url(#IDofSVGpath);

// Doesn't match correctly
@import url(data:image/png;base64,iRxVB0);

Maybe we could already take this one into account?

@@ -1112,7 +1112,7 @@ export async function hoistAtRules(css: string) {
// CSS @import can only appear at top of the file. We need to hoist all @import
// to top when multiple files are concatenated.
// match until semicolon that's not in quotes
s.replace(/@import\s*(?:"[^"]*"|'[^']*'|[^;]*).*?;/gm, (match) => {
s.replace(/@import\s*(?:url\()?(?:"[^"]*"|'[^']*'|[^;]*).*?;/gm, (match) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we keep the same pattern?

Suggested change
s.replace(/@import\s*(?:url\()?(?:"[^"]*"|'[^']*'|[^;]*).*?;/gm, (match) => {
s.replace(/@import\s*(?:url\([^\)]*\)|"[^"]*"|'[^']*'|[^;]*).*?;/gm, (match) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just saw your suggestion after my commit 72f03bc !

I used the same pattern, but not quite the same way as what you suggest. What do you prefer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I think it is better to have the url( ... ) follow the exact same pattern as the others 🤔
At least to me, it is easier to follow. If you think that both versions are doing the same, then let's go with the one I suggested. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is 4fd2810 !

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still not the same, no? What I'm proposing is to have a single non-capturing group instead of two.

Current one:

/@import\s*(?:url\()?(?:[^)]*\)|"[^"]*"|'[^']*'|[^;]*).*?;/gm

Proposal:

/@import\s*(?:url\([^\)]*\)|"[^"]*"|'[^']*'|[^;]*).*?;/gm

Or maybe I'm missing something 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied this change in chore: simplify regex 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, sorry, I misread your regexp... Thanks for the fix, I was not available for the last hour!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! Thanks again for the quick report and PR! And eager to see Histoire hit the public hopefully soon 👏🏼

@hugoattal
Copy link
Contributor Author

Thanks for the PR @hugoattal!

Thank YOU for maintaining Vite ❤️ !

@import url(data:image/png;base64,iRxVB0);

I believe it doesn't make much sense, since @import "is used to import style rules from other style sheets."
Despite the fact that this line should compile, it doesn't mean anything...

Still: here's the commit 72f03bc

@patak-dev
Copy link
Member

Merging so we can run vite-ecosystem-ci with this change

@patak-dev patak-dev merged commit a5c2a78 into vitejs:main Apr 13, 2022
@bluwy
Copy link
Member

bluwy commented Apr 14, 2022

Thanks for fixing this! My bad I missed out on this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vite build fail on css-post since 2.9.2
3 participants