-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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(importMetaGlob): support sub imports pattern #12467
Conversation
Run & review this pull request in StackBlitz Codeflow. |
I think we can consider supporting read modules via sub imports path like #12221. There will be 3 glob patterns: normal pattern, alias pattern and sub imports pattern. Each pattern's module entry key has its own style. |
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.
LGTM. In the future maybe we can improve the custom glob resolver to handle aliases only, but I think this is good enough for now.
EDIT: That might be what you mean in #12467 (comment), but I have no context of that PR and issue yet 😬
/ecosystem-ci run |
This comment was marked as outdated.
This comment was marked as outdated.
/ecosystem-ci run astro |
This comment was marked as outdated.
This comment was marked as outdated.
I am not familiar with astro. Can you help check out why this fails @bluwy Thanks. |
I think it's flaky tests happening, but since this is sort-of a feature too, we've pushed it to the next minor so we can test more extensively with Astro. |
@bluwy feel free to merge it when you feel it is better, let's start the 4.3 beta period 🙌 |
/ecosystem-ci run astro |
This comment was marked as outdated.
This comment was marked as outdated.
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
The current failures also fail in main, except Astro again 😭 I'm quite inclined to merge this still because Astro's CI error doesn't makes sense that it'd be affected by this PR. |
Maybe this pr triggers a bug in astro. It looks like the errors are the same between each ecosystem-run |
|
||
const resolved = normalizePath( | ||
(await resolveId(glob, importer, { | ||
custom: { 'glob-imports': isSubImportsPattern }, |
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.
@bluwy I also think we should merge this one, we have time to fix Astro if it is affecting it (but as you say, I doubt is related)
About the custom
option, maybe we should use a namespace for it to avoid collisions? Something like 'vite:import-glob:imports'
or 'vite:import-glob-imports'
?
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.
Yeah if this gets merged, I'll monitor Astro again for errors. A namespace sounds nice too, maybe we should follow the Vite plugin name we use vite:import-glob
, since there's also custom['node-resolve']
.
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
Updated the (await resolveId(glob, importer, {
custom: { 'vite:import-glob': { isSubImportsPattern } },
})) || glob, Let's move this one to the 4.4 milestone, better to avoid merging just before the release. |
Description
fix #12465
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).