feat: add base option for import.meta.glob #18510

hakshu25 · 2024-10-29T15:03:44Z

Description

fixes #17453.
Already #17625 is open, but it remained unresolved, so it was implemented.

sapphi-red

Thank you for the PR!

packages/vite/src/node/__tests__/plugins/importGlob/fixture-a/index.ts

packages/vite/src/node/__tests__/plugins/importGlob/__snapshots__/fixture.spec.ts.snap

packages/vite/src/node/plugins/importMetaGlob.ts

docs/guide/features.md

bluwy · 2024-11-01T15:44:51Z

docs/guide/features.md

+```ts
+// code produced by vite:
+const moduleBase = {
+  'dir/foo.js': () => import('./dir/foo.js'),


Shouldn't the key also start with ./ similar to without base?

You're right; it should definitely be consistent. I've made the changes.
7f0f5e7

I expect the key to be without ./ as '**/*.js' does not have ./ at the head. I think this depends on how to describe the base 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 name base.

I didn't notice this PR is indirectly supporting '**/*.js' when base 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 for suffix 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.

I think both behaviours should be equally powerful and can do the same things.

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 write import.meta.glob('**/*.js', { base: 'foo-' })['bar/a.js']. But if the base behaves as base, you cannot write that and need to write import.meta.glob('./**/*.js', { base: './' })['foo-bar/a.js'].

That said, I'm fine with going with base-like behavior.

Thanks, I was thinking in base-like and will keep the change.

I also think we should keep base as a path and not as a prefix.

I have modified the examples in the documentation to treat them as base instead of prefix.
500cb14

packages/vite/src/node/plugins/importMetaGlob.ts

…ob-import.spec.ts

packages/vite/src/node/plugins/importMetaGlob.ts

bluwy

Since we're treating base as the base path to resolve globs from, I think the implementation could be tweaked a bit as some still works a little like prefixes. I've made some comments below. Let me know if there's any parts unclear or would like help on.

Thanks for cleaning up the PR!

bluwy · 2024-11-06T16:14:04Z

packages/vite/src/node/plugins/importMetaGlob.ts

  const dir = importer ? globSafePath(dirname(importer)) : root
+  if (base) {
+    if (base.startsWith('./')) return pre + posix.join(dir, base, glob)
+    if (glob[0] === '@') {
+      const withoutAlias = /\/.+$/.exec(glob)?.[0]
+      return pre + posix.join(root, base, withoutAlias || glob)
+    }
+  }
+  glob = base ? posix.join(base, glob) : glob


Since base is now where we resolve globs from, I think what we should do instead is to update the dir here. For example, base should be used as dir if it's defined, otherwise we falllback to the existing logic. Like:

let dir if (base) { if (base.startsWith('/')) { // root-relative dir = posix.join(root, base) } else { // assume relative path from the current file dir = posix.resolve(importer, base) } } else { dir = importer ? globSafePath(dirname(importer)) : root }

We shouldn't need to update or return the glob ourselves. That would be like prefixing.

This assumes that base can only be ./foo (relative to the current file) or /src/foo (relative from the root), which I think should be sufficient for an initial implementation. We might want to validate in parseGlobOption that it can only start with ./, ../. or /.

Thanks, I've made the changes below.
f6c657d

bluwy · 2024-11-06T16:21:02Z

packages/vite/src/node/plugins/importMetaGlob.ts

@@ -412,19 +419,35 @@ export async function transformGlobImport(

          const resolvePaths = (file: string) => {
            if (!dir) {


This specific dir like my other comment may need to be updated too. In the code above it leads to:

vite/packages/vite/src/node/plugins/importMetaGlob.ts

Lines 376 to 383 in ce0eec9

const dir = isVirtual ? undefined : dirname(id)

const matches = await parseImportGlob(

code,

isVirtual ? undefined : id,

root,

resolveId,

logger,

)

It should also consider base like shown in my comment. However, we don't know about base until parseImportGlob() is called.

Maybe a refactor here could be to remove this dir entirely. parseImportGlob can return and we access from here instead:

vite/packages/vite/src/node/plugins/importMetaGlob.ts

Lines 392 to 393 in ce0eec9

matches.map(

async ({ globsResolved, isRelative, options, index, start, end }) => {

e.g.

async ({ globsResolved, isRelative, options, index, start, end, dir }) => {

Since we only need the dir within that callback. Free to explore other options to refactor this too though.

It would be great to make the dir reusable. I'll try refactoring it.

bluwy · 2024-11-06T16:22:37Z

packages/vite/src/node/plugins/importMetaGlob.ts

@@ -412,19 +419,35 @@ export async function transformGlobImport(

          const resolvePaths = (file: string) => {
            if (!dir) {
-              if (isRelative)
+              if (!options.base && isRelative)


With the updates only to dir in my comments, I believe we can then revert this part, from line 422 to 450, as I think it should automatically work then.

sapphi-red · 2024-11-07T02:12:52Z

packages/vite/src/node/__tests__/plugins/importGlob/fixture-a/index.ts

+export const aliasBase = import.meta.glob('@modules/*.ts', {
+  base: '/fixture-a/modules',
+})


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

const resolveId = (id: string) => id

const resolveId = (id) => { if (id.startsWith('@modules/')) { return normalizePath(path.resolve(root, 'modules', id.slice('@modules/'.length))) } return id }

I expect the output to be same with

import.meta.glob('@modules/*.ts')

if the alias works like above.

I think we should also test:

import.meta.glob('./*.ts', { base: '@modules', })

I expect the output to be same with

import.meta.glob('@modules/*.ts')

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 treating base this way I think. Unless we want to make base unique where, if it's specified, we'll resolve the keys relative to it?

I think we should also test:

import.meta.glob('./*.ts', { base: '@modules', })

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 the dir to resolve from, and resolving alias would have to involve resolveId, but I don't think it handles resolving dirs well, only files.

Since we're trying to figure out the dir to resolve from, and resolving alias would have to involve resolveId, but I don't think it handles resolving dirs well, only files.

We are passing glob to resolveId here.

vite/packages/vite/src/node/plugins/importMetaGlob.ts

Lines 594 to 598 in 500cb14

const resolved = normalizePath(

(await resolveId(glob, importer, {

custom: { 'vite:import-glob': { isSubImportsPattern } },

})) || glob,

)

Maybe the dirs can be resoved by something like resolveId(base + '*').slice(0, -'*'.length)?

Hmm yeah I guess if globs like @alias/*.ts can already be resolved here, then base 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.

hakshu25 added 3 commits October 29, 2024 22:34

feat: parse base path in import.meta.glob

fd262c0

feat: add support for custom base path in import.meta.glob

09a3e9c

test: add integration test for base option of glob-import

6b17fd7

sapphi-red added enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority) labels Oct 30, 2024

sapphi-red requested changes Oct 30, 2024

View reviewed changes

hakshu25 added 3 commits October 30, 2024 23:13

feat: add validation to check if the base option doesn't start with '!'

ae89d14

fix: resolved relative paths to options.base

5db0640

docs: add usage of base path for glob import

f6d73df

hakshu25 requested a review from sapphi-red October 31, 2024 15:23

sapphi-red reviewed Nov 1, 2024

View reviewed changes

docs/guide/features.md Outdated Show resolved Hide resolved

docs: remove one example

f206609

sapphi-red previously approved these changes Nov 1, 2024

View reviewed changes

patak-dev previously approved these changes Nov 1, 2024

View reviewed changes

bluwy reviewed Nov 1, 2024

View reviewed changes

hakshu25 added 2 commits November 2, 2024 22:25

fix: ensure glob import keys start with './' when using base option

7f0f5e7

docs: fix glob import paths to use base directory

1cd16f3

hakshu25 dismissed stale reviews from patak-dev and sapphi-red via 1cd16f3 November 2, 2024 13:56

fix: ensure glob import keys start with './' for base directory in gl…

51e76be

…ob-import.spec.ts

hakshu25 requested review from bluwy, patak-dev and sapphi-red November 2, 2024 14:50

bluwy reviewed Nov 4, 2024

View reviewed changes

packages/vite/src/node/plugins/importMetaGlob.ts Outdated Show resolved Hide resolved

hakshu25 added 2 commits November 5, 2024 23:09

feat: support alias in glob with options.base

b07d814

feat: support relative glob in virtual modules

2ec1175

hakshu25 requested a review from bluwy November 6, 2024 09:09

hakshu25 added 2 commits November 6, 2024 23:12

test: fix test to treat as base instead of prefix

76dc01e

docs: fix glob import example to treat as base instead of prefix

500cb14

bluwy reviewed Nov 6, 2024

View reviewed changes

sapphi-red reviewed Nov 7, 2024

View reviewed changes

hakshu25 added 2 commits November 7, 2024 23:13

fix: changed to update dir to glob to act as base instead of prefix

f6c657d

test: fix e2e test glob in base option of import glob

1a26072

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add base option for import.meta.glob #18510

feat: add base option for import.meta.glob #18510

hakshu25 commented Oct 29, 2024

sapphi-red left a comment

bluwy Nov 1, 2024

hakshu25 Nov 2, 2024

sapphi-red Nov 4, 2024

bluwy Nov 4, 2024

sapphi-red Nov 5, 2024 •

edited

Loading

hakshu25 Nov 6, 2024

patak-dev Nov 6, 2024

hakshu25 Nov 6, 2024

bluwy left a comment

bluwy Nov 6, 2024

hakshu25 Nov 13, 2024

bluwy Nov 6, 2024

hakshu25 Nov 13, 2024

bluwy Nov 6, 2024

sapphi-red Nov 7, 2024

bluwy Nov 7, 2024

sapphi-red Nov 7, 2024

bluwy Nov 7, 2024

		@@ -412,19 +419,35 @@ export async function transformGlobImport(

		const resolvePaths = (file: string) => {
		if (!dir) {

	const dir = isVirtual ? undefined : dirname(id)
	const matches = await parseImportGlob(
	code,
	isVirtual ? undefined : id,
	root,
	resolveId,
	logger,
	)

	matches.map(
	async ({ globsResolved, isRelative, options, index, start, end }) => {

	const resolved = normalizePath(
	(await resolveId(glob, importer, {
	custom: { 'vite:import-glob': { isSubImportsPattern } },
	})) \|\| glob,
	)

feat: add base option for import.meta.glob #18510

Are you sure you want to change the base?

feat: add base option for import.meta.glob #18510

Conversation

hakshu25 commented Oct 29, 2024

Description

sapphi-red left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapphi-red Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluwy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapphi-red Nov 5, 2024 •

edited

Loading