-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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(assets): allow new URL
to resolve package assets
#7837
Conversation
I've resolved this by including the assets plugin for |
Thanks for your PR. But I'm not sure if this is necessary, because the type of new URL is constructor (input: string, base?: string | URL); the second params is base, if you support the Maybe |
because the plugin only run in build mode. vite/packages/vite/src/node/build.ts Line 317 in fc37bdc
You can refer to vite/packages/vite/src/node/plugins/workerImportMetaUrl.ts Lines 149 to 155 in fc37bdc
You can also add this logic here and supplementary documentation if you like |
e833819
to
cfb8b39
Compare
As far as I can tell, this plugin is for transforming code that uses edit: I've extended support to worker URLs using |
4d4d33a
to
5c144a6
Compare
3624717
to
cb410a4
Compare
fix: #7779 |
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.
Thanks for bringing this to a great state so far. I have some suggestions and questions below. Feel free to let me know if you have any concerns!
e0d1a81
to
4821baa
Compare
All done, this is ready! |
a301ec3
to
50c19ca
Compare
@bluwy I do believe this is the case already. The resolver I created uses the project root, and then has a special fallback vite/packages/vite/src/node/plugins/assetImportMetaUrl.ts Lines 79 to 88 in 899caaf
In this way, both vite/packages/vite/src/node/server/middlewares/transform.ts Lines 118 to 147 in f6bd317
|
@bluwy anything I can do here? I should be able to find time to rebase this soon. |
5aa4769
to
053a33a
Compare
This tests existing behavior for obtaining an asset's URL from the public base path.
This test fails because the worker plugin's renderChunk hook runs its import.meta.url transformation after Rollup already has transformed it with importMeta.renderFinalMechanism.
This is ready again, would really like to see this released in the near future! |
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 for the lack of response @kherock. My brain wasn't up for recalling all the context of this PR until now 😦 Thanks for the reply previously, that make sense to me!
This LGTM. I think we can bring this in 3.1
Are the macOS tests flaky in CI? Neither of the last two runs seem to be related to these changes |
Looks like forth times a charm. I hope this isn't caused by this PR, plus the test failure logs seem unrelated. |
@kherock I think this is a great feature, but we'd need a team meeting to confirm it. It's planned for 3.2 now, but if there's any merge conflicts in the future I can help with resolving it. |
Description
This change allows modules to resolve asset URLs for assets located underneath a package path. E.g.
new URL('@scope/component/assets/logo.png', import.meta.url)
close #6918
Additional context
I Implemented this in a backwards-compatible way by using a resolver which prefers relative paths and makes the root path resolve to the public base path.
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).