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

perf: don't resolve import path, if it was already resolved #2659

Merged
merged 9 commits into from Jan 16, 2023
8 changes: 4 additions & 4 deletions packages/vite-node/src/client.ts
Expand Up @@ -213,14 +213,15 @@ export class ViteNodeRunner {
if (importee && id.startsWith(VALID_ID_PREFIX))
importee = undefined
id = normalizeRequestId(id, this.options.base)
if (!this.options.resolveId)
return [id, toFilePath(id, this.root)]
const { path, exists } = toFilePath(id, this.root)
if (!this.options.resolveId || exists)
return [id, path]
const resolved = await this.options.resolveId(id, importee)
const resolvedId = resolved
? normalizeRequestId(resolved.id, this.options.base)
: id
// to be compatible with dependencies that do not resolve id
const fsPath = resolved ? resolvedId : toFilePath(id, this.root)
const fsPath = resolved ? resolvedId : path
return [resolvedId, fsPath]
}

Expand Down Expand Up @@ -278,7 +279,6 @@ export class ViteNodeRunner {
if (id in requestStubs)
return requestStubs[id]

// eslint-disable-next-line prefer-const
let { code: transformed, externalize } = await this.options.fetchModule(id)

if (externalize) {
Expand Down
2 changes: 1 addition & 1 deletion packages/vite-node/src/server.ts
Expand Up @@ -125,7 +125,7 @@ export class ViteNodeServer {
private async _fetchModule(id: string): Promise<FetchResult> {
let result: FetchResult

const filePath = toFilePath(id, this.server.config.root)
const { path: filePath } = toFilePath(id, this.server.config.root)

const module = this.server.moduleGraph.getModuleById(id)
const timestamp = module ? module.lastHMRTimestamp : null
Expand Down
27 changes: 16 additions & 11 deletions packages/vite-node/src/utils.ts
Expand Up @@ -61,27 +61,32 @@ export function isPrimitive(v: any) {
return v !== Object(v)
}

export function toFilePath(id: string, root: string): string {
let absolute = (() => {
export function toFilePath(id: string, root: string): { path: string; exists: boolean } {
let { absolute, exists } = (() => {
if (id.startsWith('/@fs/'))
return id.slice(4)
return { absolute: id.slice(4), exists: true }
// check if /src/module.js -> <root>/src/module.js
if (!id.startsWith(root) && id.startsWith('/')) {
const resolved = resolve(root, id.slice(1))
// The resolved path can have query values. Remove them before checking
// the file path.
if (existsSync(resolved.replace(/\?.*$/, '')))
return resolved
if (existsSync(cleanUrl(resolved)))
return { absolute: resolved, exists: true }
}
return id
else if (id.startsWith(root) && existsSync(cleanUrl(id))) {
return { absolute: id, exists: true }
}
return { absolute: id, exists: false }
})()

if (absolute.startsWith('//'))
absolute = absolute.slice(1)

// disambiguate the `<UNIT>:/` on windows: see nodejs/node#31710
return isWindows && absolute.startsWith('/')
? slash(fileURLToPath(pathToFileURL(absolute.slice(1)).href))
: absolute
return {
path: isWindows && absolute.startsWith('/')
? slash(fileURLToPath(pathToFileURL(absolute.slice(1)).href))
: absolute,
exists,
}
}

/**
Expand Down
16 changes: 8 additions & 8 deletions test/core/test/file-path.test.ts
Expand Up @@ -82,7 +82,7 @@ describe('toFilePath', () => {
const expected = 'C:/path/to/project/node_modules/pkg/file.js'

const processSpy = vi.spyOn(process, 'cwd').mockReturnValue(root)
const filePath = toFilePath(id, root)
const { path: filePath } = toFilePath(id, root)
processSpy.mockRestore()

expect(slash(filePath)).toEqual(expected)
Expand All @@ -94,7 +94,7 @@ describe('toFilePath', () => {
const expected = 'C:/path/to/project/node_modules/pkg/file.js'

const processSpy = vi.spyOn(process, 'cwd').mockReturnValue(root)
const filePath = toFilePath(id, root)
const { path: filePath } = toFilePath(id, root)
processSpy.mockRestore()

expect(slash(filePath)).toEqual(expected)
Expand All @@ -110,7 +110,7 @@ describe('toFilePath', () => {

const processSpy = vi.spyOn(process, 'cwd').mockReturnValue(root)
const existsSpy = vi.mocked(existsSync).mockReturnValue(true)
const filePath = toFilePath(id, root)
const { path: filePath } = toFilePath(id, root)
processSpy.mockRestore()
existsSpy.mockRestore()

Expand All @@ -124,7 +124,7 @@ describe('toFilePath', () => {

const processSpy = vi.spyOn(process, 'cwd').mockReturnValue(root)
const existsSpy = vi.mocked(existsSync).mockReturnValue(true)
const filePath = toFilePath(id, root)
const { path: filePath } = toFilePath(id, root)
processSpy.mockRestore()
existsSpy.mockRestore()

Expand All @@ -138,7 +138,7 @@ describe('toFilePath', () => {

const processSpy = vi.spyOn(process, 'cwd').mockReturnValue(root)
const existsSpy = vi.mocked(existsSync).mockReturnValue(true)
const filePath = toFilePath(id, root)
const { path: filePath } = toFilePath(id, root)
processSpy.mockRestore()
existsSpy.mockRestore()

Expand All @@ -152,7 +152,7 @@ describe('toFilePath', () => {

const processSpy = vi.spyOn(process, 'cwd').mockReturnValue(root)
const existsSpy = vi.mocked(existsSync).mockReturnValue(true)
const filePath = toFilePath(id, root)
const { path: filePath } = toFilePath(id, root)
processSpy.mockRestore()
existsSpy.mockRestore()

Expand All @@ -166,7 +166,7 @@ describe('toFilePath', () => {

const processSpy = vi.spyOn(process, 'cwd').mockReturnValue(root)
const existsSpy = vi.mocked(existsSync).mockReturnValue(true)
const filePath = toFilePath(id, root)
const { path: filePath } = toFilePath(id, root)
processSpy.mockRestore()
existsSpy.mockRestore()

Expand All @@ -179,7 +179,7 @@ describe('toFilePath', () => {

const processSpy = vi.spyOn(process, 'cwd').mockReturnValue(root)
const existsSpy = vi.mocked(existsSync).mockReturnValue(false)
const filePath = toFilePath(id, root)
const { path: filePath } = toFilePath(id, root)
processSpy.mockRestore()
existsSpy.mockRestore()

Expand Down
11 changes: 10 additions & 1 deletion test/core/test/imports.test.ts
Expand Up @@ -28,7 +28,7 @@ test('dynamic aliased import works', async () => {
expect(stringTimeoutMod).toBe(variableTimeoutMod)
})

test('dynamic absolute import works', async () => {
test('dynamic absolute from root import works', async () => {
const stringTimeoutMod = await import('./../src/timeout')

const timeoutPath = '/src/timeout'
Expand All @@ -37,6 +37,15 @@ test('dynamic absolute import works', async () => {
expect(stringTimeoutMod).toBe(variableTimeoutMod)
})

test('dynamic absolute with extension mport works', async () => {
const stringTimeoutMod = await import('./../src/timeout')

const timeoutPath = '/src/timeout.ts'
const variableTimeoutMod = await import(timeoutPath)

expect(stringTimeoutMod).toBe(variableTimeoutMod)
})

test('data with dynamic import works', async () => {
const dataUri = 'data:text/javascript;charset=utf-8,export default "hi"'
const { default: hi } = await import(dataUri)
Expand Down
3 changes: 3 additions & 0 deletions test/vite-node/test/server.test.ts
Expand Up @@ -10,6 +10,9 @@ describe('server works correctly', async () => {
config: {
root: '/',
},
moduleGraph: {
idToModuleMap: new Map(),
},
} as any, {
transformMode: {
web: [/web/],
Expand Down