-
-
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: add base option for import.meta.glob #18510
base: main
Are you sure you want to change the base?
Changes from all 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
Diff view
Diff view
There are no files selected for viewing
sapphi-red marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -117,6 +117,7 @@ const knownOptions = { | |||||||||||||||||||||
import: ['string'], | ||||||||||||||||||||||
exhaustive: ['boolean'], | ||||||||||||||||||||||
query: ['object', 'string'], | ||||||||||||||||||||||
base: ['string'], | ||||||||||||||||||||||
sapphi-red marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
const forceDefaultAs = ['raw', 'url'] | ||||||||||||||||||||||
|
@@ -162,6 +163,21 @@ function parseGlobOptions( | |||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
if (opts.base) { | ||||||||||||||||||||||
if (opts.base[0] === '!') { | ||||||||||||||||||||||
throw err('Option "base" cannot start with "!"', optsStartIndex) | ||||||||||||||||||||||
} else if ( | ||||||||||||||||||||||
opts.base[0] !== '/' && | ||||||||||||||||||||||
!opts.base.startsWith('./') && | ||||||||||||||||||||||
!opts.base.startsWith('../') | ||||||||||||||||||||||
) { | ||||||||||||||||||||||
throw err( | ||||||||||||||||||||||
`Option "base" must start with '/', './' or '../', but got "${opts.base}"`, | ||||||||||||||||||||||
optsStartIndex, | ||||||||||||||||||||||
) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
if (typeof opts.query === 'object') { | ||||||||||||||||||||||
for (const key in opts.query) { | ||||||||||||||||||||||
const value = opts.query[key] | ||||||||||||||||||||||
|
@@ -306,7 +322,9 @@ export async function parseImportGlob( | |||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
const globsResolved = await Promise.all( | ||||||||||||||||||||||
globs.map((glob) => toAbsoluteGlob(glob, root, importer, resolveId)), | ||||||||||||||||||||||
globs.map((glob) => | ||||||||||||||||||||||
toAbsoluteGlob(glob, root, importer, resolveId, options.base), | ||||||||||||||||||||||
), | ||||||||||||||||||||||
) | ||||||||||||||||||||||
const isRelative = globs.every((i) => '.!'.includes(i[0])) | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
@@ -412,19 +430,35 @@ export async function transformGlobImport( | |||||||||||||||||||||
|
||||||||||||||||||||||
const resolvePaths = (file: string) => { | ||||||||||||||||||||||
if (!dir) { | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This specific vite/packages/vite/src/node/plugins/importMetaGlob.ts Lines 376 to 383 in ce0eec9
It should also consider Maybe a refactor here could be to remove this vite/packages/vite/src/node/plugins/importMetaGlob.ts Lines 392 to 393 in ce0eec9
e.g. async ({ globsResolved, isRelative, options, index, start, end, dir }) => { Since we only need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be great to make the |
||||||||||||||||||||||
if (isRelative) | ||||||||||||||||||||||
if (!options.base && isRelative) | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the updates only to |
||||||||||||||||||||||
throw new Error( | ||||||||||||||||||||||
"In virtual modules, all globs must start with '/'", | ||||||||||||||||||||||
) | ||||||||||||||||||||||
const filePath = `/${relative(root, file)}` | ||||||||||||||||||||||
return { filePath, importPath: filePath } | ||||||||||||||||||||||
const importPath = `/${relative(root, file)}` | ||||||||||||||||||||||
let filePath = options.base | ||||||||||||||||||||||
? `${relative(posix.join(root, options.base), file)}` | ||||||||||||||||||||||
: importPath | ||||||||||||||||||||||
if (options.base && filePath[0] !== '.') { | ||||||||||||||||||||||
filePath = `./${filePath}` | ||||||||||||||||||||||
} | ||||||||||||||||||||||
return { filePath, importPath } | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
let importPath = relative(dir, file) | ||||||||||||||||||||||
if (importPath[0] !== '.') importPath = `./${importPath}` | ||||||||||||||||||||||
|
||||||||||||||||||||||
let filePath: string | ||||||||||||||||||||||
if (isRelative) { | ||||||||||||||||||||||
if (options.base) { | ||||||||||||||||||||||
const resolvedBasePath = options.base[0] === '/' ? root : dir | ||||||||||||||||||||||
filePath = relative( | ||||||||||||||||||||||
posix.join(resolvedBasePath, options.base), | ||||||||||||||||||||||
file, | ||||||||||||||||||||||
) | ||||||||||||||||||||||
if (filePath[0] !== '.') filePath = `./${filePath}` | ||||||||||||||||||||||
if (options.base[0] === '/') { | ||||||||||||||||||||||
importPath = `/${relative(root, file)}` | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} else if (isRelative) { | ||||||||||||||||||||||
filePath = importPath | ||||||||||||||||||||||
} else { | ||||||||||||||||||||||
filePath = relative(root, file) | ||||||||||||||||||||||
|
@@ -544,14 +578,32 @@ export async function toAbsoluteGlob( | |||||||||||||||||||||
root: string, | ||||||||||||||||||||||
importer: string | undefined, | ||||||||||||||||||||||
resolveId: IdResolver, | ||||||||||||||||||||||
base?: string, | ||||||||||||||||||||||
): Promise<string> { | ||||||||||||||||||||||
let pre = '' | ||||||||||||||||||||||
if (glob[0] === '!') { | ||||||||||||||||||||||
pre = '!' | ||||||||||||||||||||||
glob = glob.slice(1) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
root = globSafePath(root) | ||||||||||||||||||||||
const dir = importer ? globSafePath(dirname(importer)) : root | ||||||||||||||||||||||
let dir | ||||||||||||||||||||||
if (base) { | ||||||||||||||||||||||
if (base.startsWith('/')) { | ||||||||||||||||||||||
dir = posix.join(root, base) | ||||||||||||||||||||||
} else { | ||||||||||||||||||||||
dir = posix.resolve( | ||||||||||||||||||||||
importer ? globSafePath(dirname(importer)) : root, | ||||||||||||||||||||||
base, | ||||||||||||||||||||||
) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} else { | ||||||||||||||||||||||
dir = importer ? globSafePath(dirname(importer)) : root | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
if (base && glob[0] === '@') { | ||||||||||||||||||||||
const withoutAlias = /\/.+$/.exec(glob)?.[0] | ||||||||||||||||||||||
return pre + posix.join(root, base, withoutAlias || glob) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
if (glob[0] === '/') return pre + posix.join(root, glob.slice(1)) | ||||||||||||||||||||||
if (glob.startsWith('./')) return pre + posix.join(dir, glob.slice(2)) | ||||||||||||||||||||||
if (glob.startsWith('../')) return pre + posix.join(dir, glob) | ||||||||||||||||||||||
|
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.
To test this correctly, this
resolveId
function needs to have an alias feature supported.vite/packages/vite/src/node/__tests__/plugins/importGlob/fixture.spec.ts
Line 11 in 500cb14
I expect the output to be same with
if the alias works like above.
I think we should also test:
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. I was actually suggesting a different output before (#18510 (comment)). Since
base
is now where we resolve the globs from, but aliases will always return a root-relative key, that might be one of the downsides if treatingbase
this way I think. Unless we want to makebase
unique where, if it's specified, we'll resolve the keys relative to it?The implementation I had in mind when I added my reviews before is that
base: '@modules'
is a little tricky to support, and we can only support those that starts with/
or./
or../
. Since we're trying to figure out thedir
to resolve from, and resolving alias would have to involveresolveId
, but I don't think it handles resolving dirs well, only files.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.
We are passing
glob
toresolveId
here.vite/packages/vite/src/node/plugins/importMetaGlob.ts
Lines 594 to 598 in 500cb14
Maybe the dirs can be resoved by something like
resolveId(base + '*').slice(0, -'*'.length)
?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.
Hmm yeah I guess if globs like
@alias/*.ts
can already be resolved here, thenbase
should be able to do the same here too. Though if it's harder to get it right I was thinking it could be implemented later in the future. But fine by me either ways.