Skip to content
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

fix: asset new URL(,import.meta.url) match #18194

Merged

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Sep 25, 2024

Description

Glob generated from new URL(, import.meta.url) was wrong.

For example, new URL(`./foo/${foo}.js`) was tranformed into ./foo/**.js and this glob does not match ./foo/bar/baz.js and only matches JS files in foo directory.

These are the examples of how some glob matches:

# /foo/**.js
- /foo/foo.js: true
- /foo/dir/foo.js: false

# /foo/**/*.js
- /foo/foo.js: true
- /foo/dir/foo.js: true

# /foo/****.js
- /foo/foo.js: true
- /foo/dir/foo.js: false

# /foo/**/foo.js
- /foo/foo.js: true
- /foo/dir/foo.js: true

# /foo/****/foo.js
- /foo/foo.js: false
- /foo/dir/foo.js: false

https://stackblitz.com/edit/node-b5czun?file=index.js

@sapphi-red sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Sep 25, 2024
Copy link

stackblitz bot commented Sep 25, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

expect(
await transform('new URL(`./foo/${dir}.js`, import.meta.url)'),
).toMatchInlineSnapshot(
`"new URL((import.meta.glob("./foo/**/*.js", {"eager":true,"import":"default","query":"?url"}))[\`./foo/\${dir}.js\`], import.meta.url)"`,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this PR, this was ./foo/**.js.

expect(
await transform('new URL(`./foo/${dir}${file}.js`, import.meta.url)'),
).toMatchInlineSnapshot(
`"new URL((import.meta.glob("./foo/**/*.js", {"eager":true,"import":"default","query":"?url"}))[\`./foo/\${dir}\${file}.js\`], import.meta.url)"`,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this PR, this was ./foo/****.js.

'new URL(`./foo/${dir}${dir2}/index.js`, import.meta.url)',
),
).toMatchInlineSnapshot(
`"new URL((import.meta.glob("./foo/**/index.js", {"eager":true,"import":"default","query":"?url"}))[\`./foo/\${dir}\${dir2}/index.js\`], import.meta.url)"`,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this PR, this was ./foo/****/index.js.

@sapphi-red sapphi-red changed the title fix: asset new URL(,import.meta.url match fix: asset new URL(,import.meta.url) match Sep 25, 2024
@hi-ogawa
Copy link
Collaborator

Is it intended for new URL(`./foo/${key}.js`) to match nested ones like ./foo/bar/baz.js? I'm comparing it with dynamic import variable like import("./foo/${key}.js") and this seems to only match ./foo/bar.js.

@sapphi-red
Copy link
Member Author

I made it match with nested files as I thought that was the intention of the original code. But I'm fine with making it only matching non-nested files.

@hi-ogawa
Copy link
Collaborator

I'm not sure about the history, but matching only non-nested makes more sense to me as I assume that aligns closer with dynamic import var and also it's less likely to break existing usages.

@sapphi-red
Copy link
Member Author

I've updated to use * instead of **. Though I said it's not a breaking change in the team meeting, I noticed that this is actually a breaking change as new URL(`./foo/${foo}/index.js`, import.meta.url) no longer matches ./foo/bar/baz/index.js.

@sapphi-red
Copy link
Member Author

The test was failing because of a different reason (It was not caused by the ** -> * change). It should be passing now.

@patak-dev patak-dev merged commit 5286a90 into vitejs:main Oct 30, 2024
15 checks passed
@sapphi-red sapphi-red deleted the fix/asset-new-url-import-meta-url-match branch October 31, 2024 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants