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

add data url support #515

Merged
merged 4 commits into from Dec 7, 2022

Conversation

romainmenke
Copy link
Collaborator

This change adds support for :

@import url(data:text/css;base64,...)

Before this change a data url would throw an error, so this is not something that is being used today by users of this plugin.

As far as I can tell the change required is minimal and everything just works.

  • media queries
  • at layer
  • sourcemaps
  • nesting multiple imports within data urls

I don't this is useful on its own.
Authors should not be base64 encoding CSS source code and storing this in @import urls.

It can however be useful for other plugins that have non-standard interactions with @import.

Another plugin that does want to provide extra features can choose to store as base64 instead of inlining.

This keeps the source valid with all @import rules before anything else and all in the correct order.


An alternative approach to this is that other plugins use the file system.
Instead of inlining they can store the contents in a file and update the @import rules so they each is fully standard CSS and everything points to a local file.

Both approaches have merit and I don't have a strong preference between them.

Copy link
Collaborator

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

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

Implementation looks good

lib/data-url.js Outdated Show resolved Hide resolved
@RyanZim
Copy link
Collaborator

RyanZim commented Dec 6, 2022

An alternative approach to this is that other plugins use the file system.
Instead of inlining they can store the contents in a file and update the @import rules so they each is fully standard CSS and everything points to a local file.

The problem with this approach is that you'd need to do an absolute path to a temporary file, which will create a dependency message for that file, causing runners to watch those files in watch mode, which is undesirable. I think the data URL approach should be preferred for that reason.

Not sure if anyone will use it, but it's not complicated to add, so no real reason not to.

Co-authored-by: Ryan Zimmerman <opensrc@ryanzim.com>
@romainmenke
Copy link
Collaborator Author

The problem with this approach is that you'd need to do an absolute path to a temporary file, which will create a dependency message for that file, causing runners to watch those files in watch mode, which is undesirable. I think the data URL approach should be preferred for that reason.

Cool! Thank you reviewing this 🙇

@romainmenke romainmenke merged commit 90e035b into master Dec 7, 2022
@romainmenke romainmenke deleted the add-data-url-support--intuitive-dodo-e728943167 branch December 7, 2022 17:31
@RyanZim
Copy link
Collaborator

RyanZim commented Dec 7, 2022

Published in postcss-import@15.1.0

@romainmenke
Copy link
Collaborator Author

Thank you for the release @RyanZim 🙇

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

Successfully merging this pull request may close these issues.

None yet

2 participants