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

Error on embedded imports in data urls #543

Conversation

romainmenke
Copy link
Collaborator

@romainmenke romainmenke commented Aug 9, 2023

The current code prevents further inlining of embedded imports but it doesn't stop a browser from possible loading and applying it.

Adding not all has the intended effect.

Setting stmt.children = [] prevents inlining and removes the @import statement.

@RyanZim
Copy link
Collaborator

RyanZim commented Aug 10, 2023

If such imports are completely useless, why do we output useless imports vs. erroring?

@romainmenke
Copy link
Collaborator Author

A warning would help users to write more effective CSS.

But a hard error might be too much as this is perfectly valid CSS, it's just CSS that isn't applied.

This is similar to the warning for empty files.
Empty files are valid, just not very useful.

@romainmenke
Copy link
Collaborator Author

romainmenke commented Aug 10, 2023

I've added a suggestion for a warning, please let me know if this is clear.


An alternative would be to warn and completely remove the @import node.

But that would require a larger refactor, adding not all is a non-intrusive and technically correct way to ignore these @import statements.


Update :

I found that setting stmt.children to an empty array is an effective way to prevent inlining.

@@ -1,6 +1,28 @@
@import url(foo.css);p { color: green; }p { color: blue; }@media (min-width: 320px){@layer foo{
p { color: green; } } }@media (min-width: 320px){@layer foo{
p { color: green; }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change affects the output format because the first node is removed.

@RyanZim
Copy link
Collaborator

RyanZim commented Sep 30, 2023

Needs rebase.

But a hard error might be too much as this is perfectly valid CSS, it's just CSS that isn't applied.

In other words, you're saying browsers ignore imports like these? I'm not sure if that's a valid premise to build on, because browsers will continue to parse the CSS they can, even if an import results in a 404; however, as a build tool, we'd be remiss not to error out if we can't resolve an import.

@romainmenke romainmenke force-pushed the properly-ignore-embeded-imports-in-data-urls--sensible-squid-f894715b8a branch from 89500c1 to 17a32a6 Compare September 30, 2023 17:32
@romainmenke
Copy link
Collaborator Author

romainmenke commented Sep 30, 2023

we'd be remiss not to error out if we can't resolve an import.

Makes sense!
I've changed it to an error.

Thank you for reviewing these 🙇

@RyanZim RyanZim changed the title properly ignore embedded imports in data urls Error on embedded imports in data urls Oct 2, 2023
@RyanZim RyanZim merged commit 6be601c into master Oct 2, 2023
3 checks passed
@RyanZim RyanZim deleted the properly-ignore-embeded-imports-in-data-urls--sensible-squid-f894715b8a branch October 2, 2023 16:59
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.

None yet

2 participants