Skip to content

Commit

Permalink
fix: don't resolve builtin Node modules (#2538)
Browse files Browse the repository at this point in the history
* fix: use "node:" prefix in Vitest imports

* chore: add test for prefix

* chore: cleanup

* chore: use more node protocol

* chore: cleanup

* chore: cleanup
  • Loading branch information
sheremet-va committed Dec 20, 2022
1 parent 7ae1417 commit 1cbc24d
Show file tree
Hide file tree
Showing 39 changed files with 112 additions and 60 deletions.
1 change: 1 addition & 0 deletions packages/browser/rollup.config.js
Expand Up @@ -10,6 +10,7 @@ const external = [
...Object.keys(pkg.dependencies),
...Object.keys(pkg.peerDependencies || {}),
'worker_threads',
'node:worker_threads',
]

const plugins = [
Expand Down
1 change: 1 addition & 0 deletions packages/ui/rollup.config.js
Expand Up @@ -12,6 +12,7 @@ const external = [
...Object.keys(pkg.dependencies),
...Object.keys(pkg.peerDependencies || {}),
'worker_threads',
'node:worker_threads',
'vitest/node',
'vitest',
]
Expand Down
20 changes: 12 additions & 8 deletions packages/vite-node/rollup.config.js
Expand Up @@ -26,16 +26,11 @@ const external = [
'pathe',
'birpc',
'vite',
'url',
'events',
'node:url',
'node:events',
]

const plugins = [
alias({
entries: [
{ find: /^node:(.+)$/, replacement: '$1' },
],
}),
resolve({
preferBuiltins: true,
}),
Expand Down Expand Up @@ -68,7 +63,16 @@ export default defineConfig([
chunkFileNames: 'chunk-[name].cjs',
},
external,
plugins,
plugins: [
alias({
entries: [
// cjs in Node 14 doesn't support node: prefix
// can be dropped, when we drop support for Node 14
{ find: /^node:(.+)$/, replacement: '$1' },
],
}),
...plugins,
],
onwarn,
},
{
Expand Down
16 changes: 10 additions & 6 deletions packages/vite-node/src/client.ts
@@ -1,9 +1,9 @@
import { createRequire } from 'module'
import { createRequire } from 'node:module'
// we need native dirname, because windows __dirname has \\
// eslint-disable-next-line no-restricted-imports
import { dirname } from 'path'
import { fileURLToPath, pathToFileURL } from 'url'
import vm from 'vm'
import { dirname } from 'node:path'
import { fileURLToPath, pathToFileURL } from 'node:url'
import vm from 'node:vm'
import { isNodeBuiltin } from 'mlly'
import { resolve } from 'pathe'
import createDebug from 'debug'
import { VALID_ID_PREFIX, cleanUrl, isInternalRequest, isPrimitive, normalizeModuleId, normalizeRequestId, slash, toFilePath } from './utils'
Expand Down Expand Up @@ -190,8 +190,12 @@ export class ViteNodeRunner {
}
}

shouldResolveId(id: string, _importee?: string) {
return !isInternalRequest(id) && !isNodeBuiltin(id)
}

async resolveUrl(id: string, importee?: string): Promise<[url: string, fsPath: string]> {
if (isInternalRequest(id))
if (!this.shouldResolveId(id))
return [id, id]
// we don't pass down importee here, because otherwise Vite doesn't resolve it correctly
if (importee && id.startsWith(VALID_ID_PREFIX))
Expand Down
2 changes: 1 addition & 1 deletion packages/vite-node/src/debug.ts
@@ -1,5 +1,5 @@
/* eslint-disable no-console */
import { existsSync, promises as fs } from 'fs'
import { existsSync, promises as fs } from 'node:fs'
import { join, resolve } from 'pathe'
import type { TransformResult } from 'vite'
import c from 'picocolors'
Expand Down
2 changes: 1 addition & 1 deletion packages/vite-node/src/externalize.ts
@@ -1,4 +1,4 @@
import { existsSync } from 'fs'
import { existsSync } from 'node:fs'
import { isNodeBuiltin, isValidNodeImport } from 'mlly'
import type { DepsHandlingOptions } from './types'
import { slash } from './utils'
Expand Down
2 changes: 1 addition & 1 deletion packages/vite-node/src/hmr/emitter.ts
@@ -1,4 +1,4 @@
import { EventEmitter } from 'events'
import { EventEmitter } from 'node:events'
import type { HMRPayload, Plugin } from 'vite'

export type EventType = string | symbol
Expand Down
2 changes: 1 addition & 1 deletion packages/vite-node/src/server.ts
@@ -1,4 +1,4 @@
import { performance } from 'perf_hooks'
import { performance } from 'node:perf_hooks'
import { resolve } from 'pathe'
import type { TransformResult, ViteDevServer } from 'vite'
import createDebug from 'debug'
Expand Down
7 changes: 4 additions & 3 deletions packages/vite-node/src/utils.ts
@@ -1,5 +1,5 @@
import { fileURLToPath, pathToFileURL } from 'url'
import { existsSync } from 'fs'
import { fileURLToPath, pathToFileURL } from 'node:url'
import { existsSync } from 'node:fs'
import { resolve } from 'pathe'
import type { Arrayable, Nullable } from './types'

Expand All @@ -23,7 +23,7 @@ export function normalizeRequestId(id: string, base?: string): string {
.replace(/^\/@id\/__x00__/, '\0') // virtual modules start with `\0`
.replace(/^\/@id\//, '')
.replace(/^__vite-browser-external:/, '')
.replace(/^(node|file):/, '')
.replace(/^file:/, '')
.replace(/^\/+/, '/') // remove duplicate leading slashes
.replace(/\?v=\w+/, '?') // remove ?v= query
.replace(/&v=\w+/, '') // remove &v= query
Expand All @@ -50,6 +50,7 @@ export function normalizeModuleId(id: string) {
.replace(/\\/g, '/')
.replace(/^\/@fs\//, isWindows ? '' : '/')
.replace(/^file:\//, '/')
.replace(/^node:/, '')
.replace(/^\/+/, '/')
}

Expand Down
11 changes: 2 additions & 9 deletions packages/vitest/rollup.config.js
Expand Up @@ -6,7 +6,6 @@ import dts from 'rollup-plugin-dts'
import nodeResolve from '@rollup/plugin-node-resolve'
import commonjs from '@rollup/plugin-commonjs'
import json from '@rollup/plugin-json'
import alias from '@rollup/plugin-alias'
import license from 'rollup-plugin-license'
import c from 'picocolors'
import fg from 'fast-glob'
Expand Down Expand Up @@ -41,6 +40,8 @@ const external = [
...Object.keys(pkg.dependencies),
...Object.keys(pkg.peerDependencies),
'worker_threads',
'node:worker_threads',
'node:fs',
'inspector',
'vite-node/source-map',
'vite-node/client',
Expand All @@ -49,14 +50,6 @@ const external = [
]

const plugins = [
alias({
entries: [
{ find: /^node:(.+)$/, replacement: '$1' },
{ find: 'vite-node/server', replacement: resolve(__dirname, '../vite-node/src/server.ts') },
{ find: 'vite-node/client', replacement: resolve(__dirname, '../vite-node/src/client.ts') },
{ find: 'vite-node/utils', replacement: resolve(__dirname, '../vite-node/src/utils.ts') },
],
}),
nodeResolve({
preferBuiltins: true,
}),
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/src/api/setup.ts
@@ -1,4 +1,4 @@
import { promises as fs } from 'fs'
import { promises as fs } from 'node:fs'
import type { BirpcReturn } from 'birpc'
import { createBirpc } from 'birpc'
import { parse, stringify } from 'flatted'
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/src/constants.ts
@@ -1,4 +1,4 @@
import url from 'url'
import url from 'node:url'
import { resolve } from 'pathe'
import { isNode } from './utils/env'

Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/src/integrations/env/node.ts
@@ -1,4 +1,4 @@
import { Console } from 'console'
import { Console } from 'node:console'
import type { Environment } from '../../types'

export default <Environment>({
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/src/integrations/snapshot/port/state.ts
Expand Up @@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

import fs from 'fs'
import fs from 'node:fs'
import type { OptionsReceived as PrettyFormatOptions } from 'pretty-format'
import type { ParsedStack, SnapshotData, SnapshotMatchOptions, SnapshotResult, SnapshotStateOptions, SnapshotUpdateState } from '../../../types'
import { slash } from '../../../utils'
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/src/integrations/snapshot/port/utils.ts
Expand Up @@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

import fs from 'fs'
import fs from 'node:fs'
import { dirname, join } from 'pathe'
import naturalCompare from 'natural-compare'
import type { OptionsReceived as PrettyFormatOptions } from 'pretty-format'
Expand Down
4 changes: 2 additions & 2 deletions packages/vitest/src/node/cache/files.ts
@@ -1,5 +1,5 @@
import fs from 'fs'
import type { Stats } from 'fs'
import fs from 'node:fs'
import type { Stats } from 'node:fs'

type FileStatsCache = Pick<Stats, 'size'>

Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/src/node/cache/index.ts
@@ -1,4 +1,4 @@
import fs from 'fs'
import fs from 'node:fs'
import { findUp } from 'find-up'
import { resolve } from 'pathe'
import { loadConfigFromFile } from 'vite'
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/src/node/cache/results.ts
@@ -1,4 +1,4 @@
import fs from 'fs'
import fs from 'node:fs'
import { dirname, resolve } from 'pathe'
import type { File, ResolvedConfig } from '../../types'
import { version } from '../../../package.json'
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/src/node/core.ts
@@ -1,4 +1,4 @@
import { existsSync, promises as fs } from 'fs'
import { existsSync, promises as fs } from 'node:fs'
import type { ViteDevServer } from 'vite'
import { normalizePath } from 'vite'
import { relative, toNamespacedPath } from 'pathe'
Expand Down
6 changes: 3 additions & 3 deletions packages/vitest/src/node/pool.ts
@@ -1,6 +1,6 @@
import { MessageChannel } from 'worker_threads'
import _url from 'url'
import { cpus } from 'os'
import { MessageChannel } from 'node:worker_threads'
import _url from 'node:url'
import { cpus } from 'node:os'
import { resolve } from 'pathe'
import type { Options as TinypoolOptions } from 'tinypool'
import { Tinypool } from 'tinypool'
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/src/node/reporters/benchmark/json.ts
@@ -1,4 +1,4 @@
import { existsSync, promises as fs } from 'fs'
import { existsSync, promises as fs } from 'node:fs'
import { dirname, resolve } from 'pathe'
import type { Vitest } from '../../../node'
import type { BenchTaskResult, File, Reporter } from '../../../types'
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/src/node/reporters/json.ts
@@ -1,4 +1,4 @@
import { existsSync, promises as fs } from 'fs'
import { existsSync, promises as fs } from 'node:fs'
import { dirname, resolve } from 'pathe'
import type { Vitest } from '../../node'
import type { File, Reporter, Suite, Task, TaskState } from '../../types'
Expand Down
4 changes: 2 additions & 2 deletions packages/vitest/src/node/reporters/junit.ts
@@ -1,5 +1,5 @@
import { existsSync, promises as fs } from 'fs'
import { hostname } from 'os'
import { existsSync, promises as fs } from 'node:fs'
import { hostname } from 'node:os'
import { dirname, relative, resolve } from 'pathe'

import type { Vitest } from '../../node'
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/src/runtime/entry.ts
@@ -1,4 +1,4 @@
import { promises as fs } from 'fs'
import { promises as fs } from 'node:fs'
import type { EnvironmentOptions, ResolvedConfig, VitestEnvironment } from '../types'
import { getWorkerState, resetModules } from '../utils'
import { vi } from '../integrations/vi'
Expand Down
11 changes: 11 additions & 0 deletions packages/vitest/src/runtime/execute.ts
@@ -1,6 +1,8 @@
import { ViteNodeRunner } from 'vite-node/client'
import { isInternalRequest } from 'vite-node/utils'
import type { ViteNodeRunnerOptions } from 'vite-node'
import { normalizePath } from 'vite'
import { isNodeBuiltin } from 'mlly'
import type { MockMap } from '../types/mocker'
import { getCurrentEnvironment, getWorkerState } from '../utils'
import { VitestMocker } from './mocker'
Expand Down Expand Up @@ -31,6 +33,15 @@ export class VitestRunner extends ViteNodeRunner {
this.mocker = new VitestMocker(this)
}

shouldResolveId(id: string, _importee?: string | undefined): boolean {
if (isInternalRequest(id))
return false
const environment = getCurrentEnvironment()
// do not try and resolve node builtins in Node
// import('url') returns Node internal even if 'url' package is installed
return environment === 'node' ? !isNodeBuiltin(id) : true
}

async resolveUrl(id: string, importee?: string) {
if (importee && importee.startsWith('mock:'))
importee = importee.slice(5)
Expand Down
4 changes: 2 additions & 2 deletions packages/vitest/src/runtime/loader.ts
@@ -1,5 +1,5 @@
import { pathToFileURL } from 'url'
import { readFile } from 'fs/promises'
import { pathToFileURL } from 'node:url'
import { readFile } from 'node:fs/promises'
import { hasCJSSyntax, isNodeBuiltin } from 'mlly'
import { normalizeModuleId } from 'vite-node/utils'
import { getWorkerState } from '../utils'
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/src/runtime/mocker.ts
@@ -1,4 +1,4 @@
import { existsSync, readdirSync } from 'fs'
import { existsSync, readdirSync } from 'node:fs'
import { isNodeBuiltin } from 'mlly'
import { basename, dirname, extname, isAbsolute, join, resolve } from 'pathe'
import c from 'picocolors'
Expand Down
4 changes: 2 additions & 2 deletions packages/vitest/src/runtime/setup.ts
Expand Up @@ -50,8 +50,8 @@ export async function setupConsoleLogSpy() {
const timers = new Map<string, { stdoutTime: number; stderrTime: number; timer: any }>()
const unknownTestId = '__vitest__unknown_test__'

const { Writable } = await import('stream')
const { Console } = await import('console')
const { Writable } = await import('node:stream')
const { Console } = await import('node:console')

// group sync console.log calls with macro task
function schedule(taskId: string) {
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/src/typecheck/typechecker.ts
@@ -1,4 +1,4 @@
import { rm } from 'fs/promises'
import { rm } from 'node:fs/promises'
import type { ExecaChildProcess } from 'execa'
import { execa } from 'execa'
import { resolve } from 'pathe'
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/src/types/worker.ts
@@ -1,4 +1,4 @@
import type { MessagePort } from 'worker_threads'
import type { MessagePort } from 'node:worker_threads'
import type { FetchFunction, ModuleCacheMap, RawSourceMap, ViteNodeResolveId } from 'vite-node'
import type { BirpcReturn } from 'birpc'
import type { MockMap } from './mocker'
Expand Down
3 changes: 1 addition & 2 deletions packages/vitest/src/utils/index.ts
@@ -1,5 +1,4 @@
// eslint-disable-next-line no-restricted-imports
import { relative as relativeBrowser } from 'path'
import { relative as relativeBrowser } from 'node:path'
import c from 'picocolors'
import { isPackageExists } from 'local-pkg'
import { relative as relativeNode } from 'pathe'
Expand Down
2 changes: 1 addition & 1 deletion packages/web-worker/src/shared-worker.ts
@@ -1,4 +1,4 @@
import { MessageChannel, type MessagePort as NodeMessagePort } from 'worker_threads'
import { MessageChannel, type MessagePort as NodeMessagePort } from 'node:worker_threads'
import type { InlineWorkerContext, Procedure } from './types'
import { InlineWorkerRunner } from './runner'
import { debug, getRunnerOptions } from './utils'
Expand Down
2 changes: 2 additions & 0 deletions packages/ws-client/rollup.config.js
Expand Up @@ -13,7 +13,9 @@ const external = [
'ws',
'birpc',
'worker_threads',
'node:worker_threads',
'fs',
'node:fs',
'vitest',
'inspector',
]
Expand Down
2 changes: 2 additions & 0 deletions pnpm-lock.yaml

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

2 changes: 1 addition & 1 deletion scripts/update-contributors.ts
@@ -1,4 +1,4 @@
import { promises as fs } from 'fs'
import { promises as fs } from 'node:fs'
import { $fetch } from 'ohmyfetch'

interface Contributor {
Expand Down
2 changes: 1 addition & 1 deletion test/benchmark/test.mjs
@@ -1,4 +1,4 @@
import { readFile } from 'fs/promises'
import { readFile } from 'node:fs/promises'
import { execa } from 'execa'

let error
Expand Down
1 change: 1 addition & 0 deletions test/core/package.json
Expand Up @@ -7,6 +7,7 @@
},
"devDependencies": {
"tinyspy": "^1.0.2",
"url": "^0.11.0",
"vitest": "workspace:*"
}
}
18 changes: 18 additions & 0 deletions test/core/test/node-protocol-jsdom.spec.ts
@@ -0,0 +1,18 @@
// @vitest-environment jsdom

// outdated url package, which Vite will resolve to, if "url" import is used
// this should help catch bugs in source code
import packageUrl from 'url'
import nodeUrl from 'node:url'
import { expect, it } from 'vitest'

it('vitest resolves url to installed url package, but node:url to internal Node module', () => {
expect(packageUrl).not.toHaveProperty('URL')
expect(packageUrl).not.toHaveProperty('URLSearchParams')
expect(packageUrl).not.toHaveProperty('fileURLToPath')
expect(nodeUrl).toHaveProperty('URL')
expect(nodeUrl).toHaveProperty('URLSearchParams')
expect(nodeUrl).toHaveProperty('fileURLToPath')
// eslint-disable-next-line n/no-deprecated-api
expect(packageUrl.parse !== nodeUrl.parse).toBe(true)
})

0 comments on commit 1cbc24d

Please sign in to comment.