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

feat(rollup-plugin-html): resolves assets in styles #2664

Merged
merged 6 commits into from
Mar 14, 2024

Conversation

bashmish
Copy link
Member

What I did

  1. rollup-plugin-html resolves assets in styles

Copy link

changeset-bot bot commented Mar 11, 2024

🦋 Changeset detected

Latest commit: c3dc21a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@web/rollup-plugin-html Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@thepassle thepassle force-pushed the feat/rollup-plugin-html-resolves-assets-in-styles branch 2 times, most recently from 79dd835 to ea299d2 Compare March 13, 2024 12:39
@thepassle thepassle force-pushed the feat/rollup-plugin-html-resolves-assets-in-styles branch from ea299d2 to 47728ce Compare March 13, 2024 13:53
@thepassle thepassle marked this pull request as ready for review March 13, 2024 18:25
@thepassle thepassle force-pushed the feat/rollup-plugin-html-resolves-assets-in-styles branch 5 times, most recently from dc24a9d to 57c2923 Compare March 13, 2024 18:42
@thepassle thepassle force-pushed the feat/rollup-plugin-html-resolves-assets-in-styles branch from 57c2923 to 549eaf7 Compare March 13, 2024 18:43
@thepassle thepassle changed the title WIP feat(rollup-plugin-html): resolves assets in styles feat(rollup-plugin-html): resolves assets in styles Mar 13, 2024
code: asset.content,
minify: false,
visitor: {
Url: url => {
Copy link
Member Author

Choose a reason for hiding this comment

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

does this recursively follow all css files includes (the @import ones)?

if yes, looks like source = Buffer.from(code); would not update anything but the entry point

Copy link
Member

Choose a reason for hiding this comment

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

The code as currently implemented does not bundle css via @import. I looked at both esbuild and lightningcss, and they have a bundle for css, but it only follows @import, and not url, which is what we need. So for now I only added support for assets referenced via url, afaik you can't add .css files in url. In the future we could consider bundling imported css

Copy link
Member Author

Choose a reason for hiding this comment

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

clear, makes sense!

/.*\.avif/,
/.*\.woff2/,
/.*\.woff/,
];
Copy link
Member Author

Choose a reason for hiding this comment

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

why do you want to use the allowed extensions list? are there some extensions that should not be handled?

Copy link
Member

Choose a reason for hiding this comment

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

We're not really sure what people may add in url, so I figured be on the safe side

Copy link
Member Author

Choose a reason for hiding this comment

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

with current approach we'll need to maintain a list of extensions and update when new types appear or if we forgot to include smth

instead I'd rather prefer to have a blocklist and make it configurable, we can do that later too

@bashmish
Copy link
Member Author

super nice work Pascal!
I'm just concerned about 2 things

  • more about the allowed extensions list
  • less about the recursive css imports handling - this one we can also add in the future, but not sure how many need this as css nested imports is not a super nice performance strategy and might not be used much

@thepassle
Copy link
Member

@bashmish im gonna go ahead and merge this (it will take some time to end up in our internal npm feed as well, and i'd really like to wrap this up today), the other things we can do in another iteration/separate PR

@thepassle thepassle merged commit cd95ba9 into master Mar 14, 2024
6 checks passed
@thepassle thepassle deleted the feat/rollup-plugin-html-resolves-assets-in-styles branch March 14, 2024 09:14
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.

2 participants