Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: add base option for import.meta.glob #18510
base: main
Are you sure you want to change the base?
feat: add base option for import.meta.glob #18510
Changes from 7 commits
fd262c0
09a3e9c
6b17fd7
ae89d14
5db0640
f6d73df
f206609
7f0f5e7
1cd16f3
51e76be
b07d814
2ec1175
76dc01e
500cb14
f6c657d
1a26072
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Shouldn't the key also start with
./
similar to without base?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.
You're right; it should definitely be consistent. I've made the changes.
7f0f5e7
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.
I expect the key to be without
./
as'**/*.js'
does not have./
at the head. I think this depends on how to describe thebase
option. If we describe this as a prefix string, then I think we should remove./
from the key. If we describe this as a base path, then I think we should add./
to the key. A prefix string is more powerful than base path because it allows non-path prefix and maybe it works well with aliases, but the base path feels more natural with the namebase
.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.
I didn't notice this PR is indirectly supporting
'**/*.js'
whenbase
is set. I was thinking more like the latter, a base path to resolve the glob from and it feels easier to visualize (for me), e.g. as if you're resolving the glob in a different file path.I think both behaviours should be equally powerful and can do the same things. But if the behaviour is more like a prefix, then perhaps the option should be called
prefix
, but then someone could ask forsuffix
too.I also think the behaviour could be easier implemented if treated as a base path, so instead of using the file dir or root (for virtual modules), we could use whatever passed by
base
instead.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.
I think the prefix one is a bit more powerful. For the following files,
foo-bar/a.js
foo-baz/b.js
if the
base
behaves as prefix, you can writeimport.meta.glob('**/*.js', { base: 'foo-' })['bar/a.js']
. But if thebase
behaves as base, you cannot write that and need to writeimport.meta.glob('./**/*.js', { base: './' })['foo-bar/a.js']
.That said, I'm fine with going with base-like behavior.
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, I was thinking in base-like and will keep the change.
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.
I also think we should keep base as a path and not as a prefix.
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.
I have modified the examples in the documentation to treat them as base instead of prefix.
500cb14