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
Conversation
Thanks for the PR @hugoattal! I'm testing with a few more URLs from the linked MDN page:
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) => { |
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.
What if we keep the same pattern?
s.replace(/@import\s*(?:url\()?(?:"[^"]*"|'[^']*'|[^;]*).*?;/gm, (match) => { | |
s.replace(/@import\s*(?:url\([^\)]*\)|"[^"]*"|'[^']*'|[^;]*).*?;/gm, (match) => { |
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.
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?
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.
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!
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.
Here it is 4fd2810 !
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.
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 🤔
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.
Applied this change in chore: simplify regex 👍🏼
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.
Oops, sorry, I misread your regexp... Thanks for the fix, I was not available for the last hour!
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.
No problem! Thanks again for the quick report and PR! And eager to see Histoire hit the public hopefully soon 👏🏼
Thank YOU for maintaining Vite ❤️ !
I believe it doesn't make much sense, since Still: here's the commit 72f03bc |
Merging so we can run vite-ecosystem-ci with this change |
Thanks for fixing this! My bad I missed out on this case. |
fix #7717
Description
Handle CSS Url imports, example:
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?
Before submitting the PR, please make sure you do the following
fixes #123
).