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.

Sign up for GitHub

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

Jump to bottom

feat(vite-node): provide import.meta.resolve #5188

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

feat(vite-node): provide import.meta.resolve #5188

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
060a638
feat(vite-node): provide `import.meta.resolve`
hi-ogawa Feb 13, 2024
5dd2075
chore: lockfile
hi-ogawa Feb 13, 2024
bfc2f4f
chore: comment
hi-ogawa Feb 13, 2024
b4a9b72
chore: comment
hi-ogawa Feb 13, 2024
0a0b73c
test: test vite-node cjs
hi-ogawa Feb 13, 2024
74ca778
fix: check `import.meta.resolve`
hi-ogawa Feb 13, 2024
2cc0ba9
fix: define only when "parent" feature is available
hi-ogawa Feb 13, 2024
f8c05b4
fix: await to avoid unhandled rejection in old node
hi-ogawa Feb 13, 2024
c11c3ea
chore: still define but throw when called
hi-ogawa Feb 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/vite-node/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,9 @@ export class ViteNodeRunner {
env,
filename: __filename,
dirname: __dirname,
// this requires users to enable "--experimental-import-meta-resolve" even on the latest NodeJS since it uses 2nd argument `parent`.
// `import.meta.resolve` becomes `undefined` for vite-node cjs build. So this won't cause syntax error on cjs.
Copy link
Member

@sheremet-va sheremet-va Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

'resolve' in import.meta will resolve different values in Node and Vitest, right?

Copy link
Contributor Author

@hi-ogawa hi-ogawa Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For latest Node (from v20.0.6 https://nodejs.org/docs/latest-v20.x/api/esm.html#importmetaresolvespecifier), import.meta.resolve is always defined and --experimental-import-meta-resolve only switches 2nd argument parent feature. But for older node, --experimental-import-meta-resolve is required to have import.meta.resolve defined.

So, 'resolve' in import.meta should be same for the latest Node (regardless of --experimental-import-meta-resolve), but that's not the case for old ones.

I just put next to filename/dirname here, but it's probably better to define this only when import.meta.resolve is available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe you were asking some enumerable property kind of difference?

For now, I updated it to define import.meta.resolve by checking it first 74ca778

Copy link
Member

@sheremet-va sheremet-va Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Or maybe you were asking some enumerable property kind of difference?

Just defining with an if is fine for me. I do have another question tho - shouldn't it give an error on the new Node.js version when you don't pass down a flag and use a relative path because it doesn't support parent? Since all relative URL are resolved relative to the vite-node file.

Copy link
Contributor Author

@hi-ogawa hi-ogawa Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

shouldn't it give an error on the new Node.js version when you don't pass down a flag because it doesn't support parent? And all relative URL are resolved relative to the vite-node file.

Yes, that's very much desired, but I don't know how to do it. I thought about just checking execArgv, but the flag might also come from NODE_OPTIONS. Do you know some trick for this?

Another way would be to detect feature during runtime by actually testing import.meta.resolve, something like:

// this would give ".../dummy/dummy" if the 2nd argument `parent` is supported 
import.meta.resolve("./dummy", new URL("./dummy/", import.meta.url))

Copy link
Contributor Author

@hi-ogawa hi-ogawa Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

With vite-node it is resolved relative to the vite-node file instead of the current file, correct?

When users didn't pass --experimental-import-meta-resolve, that's going to happen silently, so that's why I thought we need to detect that first.

If --experimental-import-meta-resolve is passed, then the we can override the default parent value, so import.meta.resolve('./dumy') should work same both on Node and vite-node.

I updated to check if the 2nd argument feature is available by running this inside vite-node

import.meta.resolve('.', new URL('./__test_parent__/', import.meta.url))

(Actually evaluating this for every module is unnecessary, so probably we can check just once.)

Copy link
Member

@sheremet-va sheremet-va Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When users didn't pass --experimental-import-meta-resolve, that's going to happen silently, so that's why I thought we need to detect that first.

Yeah, but even if we detect it, we can't fix it with import.meta.resolve, can't we? Since it doesn't accept the second argument. Do you only want to support it if there is a flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you only want to support it if there is a flag?

Yes, that's what I was thinking. But, I totally get that exposing this feature only with --experimental-import-meta-resolve wouldn't be that good. I'm okay with that putting this PR on hold for now.

Copy link
Member

Choose a reason for hiding this comment

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

We already bundle import-meta-resolve package, we can just use it here 😄

Copy link
Contributor Author

@hi-ogawa hi-ogawa Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wrote it in vitejs/vite#15871 (comment) but there are some known differences with a polyfill https://github.com/wooorm/import-meta-resolve?tab=readme-ov-file#differences-to-node

It totally depends on the use case, but my feeling is that if users really want to integrate import.meta.resolve feature in NodeJs app/library, then they would use it together with custom loader or custom conditions etc...
(actually I had this use case on my own tiny typescript import/export checker https://github.com/hi-ogawa/js-utils/tree/main/packages/icheck-ts where it doesn't make sense to use polyfill)

If that's not the case, then they would've used createRequire + require.resolve (for node_modules) or new URL("./...", ...) (for relative path) or maybe polyfill by themselves.

I guess this feature request came for the first time, so I think we can wait a little to determine the use cases better. (I didn't even get the response from OP yet...)

resolve: (specifier: string, parent?: string | URL) => import.meta.resolve(specifier, parent ?? href),
}
const exports = Object.create(null)
Object.defineProperty(exports, Symbol.toStringTag, {
Expand Down
6 changes: 6 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions test/import-meta-resolve/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"name": "@vitest/test-import-meta-resolve",
"type": "module",
"private": true,
"scripts": {
"test": "vitest"
},
"devDependencies": {
"vitest": "workspace:*"
}
}
41 changes: 41 additions & 0 deletions test/import-meta-resolve/test/basic.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { expect, test } from 'vitest'

test('basic', () => {
// relative path
expect(
cleanDir(import.meta.resolve('../package.json')),
).toMatchInlineSnapshot(`"__DIR__/test/import-meta-resolve/package.json"`)

// not throw in latest NodeJS
expect(cleanDir(import.meta.resolve('../no-such-file'))).toMatchInlineSnapshot(
`"__DIR__/test/import-meta-resolve/no-such-file"`,
)

// with 2nd argument `parent`
expect(
cleanDir(
import.meta.resolve('./package.json', new URL('..', import.meta.url)),
),
).toMatchInlineSnapshot(`"__DIR__/test/import-meta-resolve/package.json"`)

// node_module
expect(cleanDir(import.meta.resolve('vitest'))).toMatchInlineSnapshot(
`"__DIR__/packages/vitest/dist/index.js"`,
)

expect(() =>
cleanDir(import.meta.resolve('@vitest/not-such-module')),
).toThrow(
expect.objectContaining({
message: expect.stringContaining(
'Cannot find package \'@vitest/not-such-module\' imported from',
),
}),
)
})

// make output deterministic
function cleanDir(out: string) {
const dir = new URL('../../..', import.meta.url).toString()
return out.replace(dir, '__DIR__/')
}
18 changes: 18 additions & 0 deletions test/import-meta-resolve/vitest.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { defineConfig } from 'vitest/config'

export default defineConfig({
test: {
poolOptions: {
threads: {
execArgv: ['--experimental-import-meta-resolve'],
},
forks: {
execArgv: ['--experimental-import-meta-resolve'],
},
vmThreads: {
// vmThreads already enables this flag
// execArgv: ['--experimental-import-meta-resolve'],
},
},
},
})
2 changes: 2 additions & 0 deletions test/vite-node/src/require-vite-node.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
require('vite-node/client')
require('vite-node/server')
14 changes: 14 additions & 0 deletions test/vite-node/test/cjs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { execFile } from 'node:child_process'
import { fileURLToPath } from 'node:url'
import { promisify } from 'node:util'
import { it } from 'vitest'

const execFileAsync = promisify(execFile)

it('require vite-node', async () => {
// verify `import.meta.resolve` usage doesn't cause syntax error on vite-node cjs build
await execFileAsync(
'node',
[fileURLToPath(new URL('../src/require-vite-node.cjs', import.meta.url))],
)
})
Loading