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(vite-node): externalize network imports #4987

Merged

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Jan 17, 2024

Description

As shown in the reproduction, currently network import is not externalized by vite-node and Vite's transformRequest ends up with a following error:

 FAIL  test/basic.test.ts [ test/basic.test.ts ]
Error: Failed to load url https://esm.sh/react@18.2.0 (resolved id: https://esm.sh/react@18.2.0). Does the file exist?
 ❯ loadAndTransform node_modules/vite/dist/node/chunks/dep-V3BH7oO1.js:49777:21

Vite's ssrLoadModule didn't support network imports previously but it is changed to externalize it in vitejs/vite#15599, so Vite-node side change also aligns with Vite.

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 Jan 17, 2024

Deploy Preview for fastidious-cascaron-4ded94 ready!

Name Link
🔨 Latest commit 587b2bf
🔍 Latest deploy log https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/65a7ab2dcc9df5000871f015
😎 Deploy Preview https://deploy-preview-4987--fastidious-cascaron-4ded94.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hi-ogawa hi-ogawa marked this pull request as ready for review January 17, 2024 04:07
Comment on lines +12 to +19
// not supported?
// FAIL test/basic.test.ts [ test/basic.test.ts ]
// Error: ENOENT: no such file or directory, open 'http://localhost:9602/slash@3.0.0.js'
// ❯ Object.openSync node:fs:596:3
// ❯ readFileSync node:fs:464:35
vmThreads: {
execArgv: ['--experimental-network-imports'],
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like it's not working on vmThreads. I'll investigate further to see if it's simply NodeJS limitation.

Copy link
Member

Choose a reason for hiding this comment

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

I think every import is delegated to our VM implementation, and it just doesn't support http.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint! I haven't had a chance to look at how vmThreads works yet, so I'll look around a bit to get the idea.
I suppose it's still okay even if it doesn't work on vmThreads?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we can disable this test for vm threads/forks for now.

I think it's a separate feature request (maybe create an issue for it). Shouldn't be hard to implement.

@sheremet-va
Copy link
Member

I am not sure how to contact you otherwise, but I sent an e-mail to the address listed in your GitHub profile. Please have a look when you have time 😄

@sheremet-va sheremet-va merged commit 21f5744 into vitest-dev:main Jan 17, 2024
18 of 19 checks passed
@hi-ogawa hi-ogawa deleted the fix-vite-node-externalize-network-imports branch January 17, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot import from external URLs
2 participants