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): glob patterns exclusion for external assets #2671

Conversation

JulianCataldo
Copy link
Contributor

Fixes #811

What I did

  1. Added a plugin option for providing glob patterns in order to exclude assets from being extracted.

Very useful for stuff that already lives in a public folder and are copied as-is.

Why?

extractAssets is all or nothing.

transformAssets doesn't provide a way to skip an asset.

Also related: #2631

Copy link

changeset-bot bot commented Mar 15, 2024

🦋 Changeset detected

Latest commit: 6207e41

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

@JulianCataldo JulianCataldo changed the title feat: glob patterns exclusion for external assets feat(rollup-plugin-html): glob patterns exclusion for external assets Mar 15, 2024
Copy link
Member

@bashmish bashmish left a comment

Choose a reason for hiding this comment

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

I need this also in #2677 , what are the odds right? :D

PR looks good, I think we just need a few things:

  • write a simple unit test
  • I'd rename external to externalAssets, since the whole config is flat and external can refer to anything, so better make it more specific
  • use currently installed picomatch version "picomatch": "^2.2.2", (because I see types version mismatch and I think it's more handy to do an upgrade to picomatch 4 alltogether for all deps to prevent potential incompatibilities between different tools in the whole toolchain)
  • update lock file (I think npm install should have done it, but it wasn't commited?)

can you work on this today-tomorrow? if not, I'd like to work on this myself, since I need this kinda urgently

@JulianCataldo
Copy link
Contributor Author

JulianCataldo commented Mar 18, 2024

Hey!
I am on my phone so I must keep it short.
I had troubles setting up the monorepo so my strategy was to monkey patch the dist then backport the changes to the source.
I've seen a missing lightingcss dep. too on a (unreleased?) version.

I agree on your suggestions.
I was waiting for a first feedback before engaging additional work and polishing.

If it wasn't for the monorepo setup I would have gave a stab at writing a test too, but I'm a bit lost TBH.

If you're familiar with working with this repo., maybe it's better that you continue this PR.

Cya

@bashmish
Copy link
Member

@JulianCataldo I'm familiar indeed, some parts I maintain since recently, this code is new to me, but you did most of it anyway, the rest I can do quickly, so I'll take it from here then. Thanks!

@bashmish bashmish force-pushed the feat/support-external-assets-glob-patterns branch 2 times, most recently from 3dc76e5 to 3409318 Compare March 19, 2024 08:49
@bashmish bashmish force-pushed the feat/support-external-assets-glob-patterns branch from 3409318 to 5e4af37 Compare March 19, 2024 13:45
const removeLeadingSlash = (str: string) => (str.startsWith('./') ? str.slice(2) : str);
export function createAssetPicomatchMatcher(glob?: string | string[]) {
return picomatch(glob || [], { format: removeLeadingSlash });
}
Copy link
Member

Choose a reason for hiding this comment

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

this was pretty unexpected, but the proposed workaround from the picomatch maintainer seems to be good for us
micromatch/picomatch#77

I added different variations in the test, with and without ./, to be sure it works

}

#d2 {
background-image: url('./image-d.svg');
Copy link
Member

Choose a reason for hiding this comment

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

some deduplication of images was happening, I think because we try to identify images with same hash and merge them into one, so it's a feature of this plugin
but it made testing the exclusion cumbersome, so I added a new image-d.svg/png with the content equal to the file name to get a different hash than for image-a and see in the output all expected exclusions

the deduplication continues to work correctly, it's just that test would be hard to read if I mix so much different functionality in one place

// picomatch follows glob spec and requires "./" to be removed for the matcher to work
// it is safe, because with or without it resolves to the same file
// read more: https://github.com/micromatch/picomatch/issues/77
const removeLeadingSlash = (str: string) => (str.startsWith('./') ? str.slice(2) : str);
Copy link
Member

Choose a reason for hiding this comment

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

can't we just path.normalize it?

Copy link
Member

@bashmish bashmish Mar 19, 2024

Choose a reason for hiding this comment

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

I wanted to keep modifications to the original paths used in source files to a minimum
path.normalize would solve it too, but might make other unexpected modifications which might influence the matcher behavior
not sure we should use that to be honest

@bashmish bashmish merged commit 3dd7d5c into modernweb-dev:master Mar 19, 2024
6 of 7 checks passed
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.

External does not seem to work with @web/rollup-plugin-html
4 participants