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

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

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

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Feb 13, 2024

Description

related

I'm not sure if this has been considered before. Assuming --experimental-import-meta-resolve is explicitly managed by users (and thus this feature is considered experimental as well on vite-node/vitest), this approach seems to work.

I noticed that a similar approach is already used to implement import.meta.resolve for VM external:

meta.resolve = (specifier: string, importer?: string | URL) => {
return this.executor.resolve(specifier, importer != null ? importer.toString() : mod.identifier)
}

Do you think this is a good addition?
If it's too early to start implementing, then I can create a separate issue on Vitest (or maybe just linking to Vite issue vitejs/vite#14500 is enough).

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

Copy link

netlify bot commented Feb 13, 2024

Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name Link
🔨 Latest commit c11c3ea
🔍 Latest deploy log https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/65cb33b66e382e0008c11666

@hi-ogawa hi-ogawa marked this pull request as ready for review February 13, 2024 04:34
@@ -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

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

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

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

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

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

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

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...)

@grabbou
Copy link

grabbou commented Feb 14, 2024

Thanks for this PR! I've just run into this issue and was researching whether this was a bug or a work in progress. One thing that doesn't work after applying this patch locally is mocks of virtual modules are not detected.

For example, I have vi.mock("@acme/test") package that does not exist in node_modules. Not sure whether this is something that should be handled by import.meta.resolve out of the box or not.

@sheremet-va
Copy link
Member

Thanks for this PR! I've just run into this issue and was researching whether this was a bug or a work in progress. One thing that doesn't work after applying this patch locally is mocks of virtual modules are not detected.

For example, I have vi.mock("@acme/test") package that does not exist in node_modules. Not sure whether this is something that should be handled by import.meta.resolve out of the box or not.

Virtual modules need to be handled by the config file: https://vitest.dev/guide/mocking.html#virtual-modules

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants