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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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.

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
None yet
Development

Successfully merging this pull request may close these issues.

2 participants