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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions eslint.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export default antfu(
'test/core/src/self',
'test/workspaces/results.json',
'test/reporters/fixtures/with-syntax-error.test.js',
'test/network-imports/public/[email protected]',
'examples/**/mockServiceWorker.js',
],
},
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"test:run": "vitest run -r test/core",
"test:all": "CI=true pnpm -r --stream run test --allowOnly",
"test:ci": "CI=true pnpm -r --stream --filter !test-browser --filter !test-esm --filter !test-browser run test --allowOnly",
"test:ci:vm-threads": "CI=true pnpm -r --stream --filter !test-coverage --filter !test-single-thread --filter !test-browser --filter !test-esm --filter !test-browser --filter !example-react-testing-lib-msw run test --allowOnly --pool vmThreads",
"test:ci:vm-threads": "CI=true pnpm -r --stream --filter !test-coverage --filter !test-single-thread --filter !test-browser --filter !test-esm --filter !test-network-imports --filter !test-browser --filter !example-react-testing-lib-msw run test --allowOnly --pool vmThreads",
"test:ci:no-threads": "CI=true pnpm -r --stream --filter !test-vm-threads --filter !test-coverage --filter !test-watch --filter !test-bail --filter !test-esm --filter !test-browser run test --allowOnly --pool forks",
"typecheck": "tsc -p tsconfig.check.json --noEmit",
"typecheck:why": "tsc -p tsconfig.check.json --noEmit --explainFiles > explainTypes.txt",
Expand Down
5 changes: 3 additions & 2 deletions packages/vite-node/src/externalize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,9 @@ async function _shouldExternalize(
return id

// data: should be processed by native import,
// since it is a feature of ESM
if (id.startsWith('data:'))
// since it is a feature of ESM.
// also externalize network imports since nodejs allows it when --experimental-network-imports
if (id.startsWith('data:') || /^(https?:)?\/\//.test(id))
return id

id = patchWindowsImportPath(id)
Expand Down
6 changes: 6 additions & 0 deletions pnpm-lock.yaml

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

11 changes: 11 additions & 0 deletions test/network-imports/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"name": "@vitest/test-network-imports",
"type": "module",
"private": true,
"scripts": {
"test": "vitest"
},
"devDependencies": {
"vitest": "workspace:*"
}
}
5 changes: 5 additions & 0 deletions test/network-imports/public/[email protected]

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

11 changes: 11 additions & 0 deletions test/network-imports/test/basic.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { expect, test } from 'vitest'

// @ts-expect-error network imports
import slash from 'http://localhost:9602/[email protected]'

// test without local server
// import slash from 'https://esm.sh/[email protected]'

test('network imports', () => {
expect(slash('foo\\bar')).toBe('foo/bar')
})
24 changes: 24 additions & 0 deletions test/network-imports/vitest.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { defineConfig } from 'vitest/config'

export default defineConfig({
test: {
poolOptions: {
threads: {
execArgv: ['--experimental-network-imports'],
},
forks: {
execArgv: ['--experimental-network-imports'],
},
// not supported?
// FAIL test/basic.test.ts [ test/basic.test.ts ]
// Error: ENOENT: no such file or directory, open 'http://localhost:9602/[email protected]'
// ❯ Object.openSync node:fs:596:3
// ❯ readFileSync node:fs:464:35
vmThreads: {
execArgv: ['--experimental-network-imports'],
},
Comment on lines +12 to +19
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

@sheremet-va sheremet-va Jan 17, 2024

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.

},
// let vite serve public/[email protected]
api: 9602,
},
})
Loading