Skip to content

Commit

Permalink
fix: don't use resolved paths if they don't actually exist
Browse files Browse the repository at this point in the history
Previously `toFilePath` would treat _any_ absolute path that doesn't
start with the `root` (usually the current working directory) as a
relative path, which resulted in some seriously mangled paths. Now we
actually check if that resolved path exists before committing to it.

This fixes #1864.
  • Loading branch information
simon-abbott committed Aug 17, 2022
1 parent e21c78c commit cc72d79
Show file tree
Hide file tree
Showing 12 changed files with 115 additions and 26 deletions.
20 changes: 13 additions & 7 deletions packages/vite-node/src/utils.ts
@@ -1,4 +1,5 @@
import { fileURLToPath, pathToFileURL } from 'url'
import { existsSync } from 'fs'
import { resolve } from 'pathe'
import type { TransformResult } from 'vite'
import type { Arrayable, Nullable } from './types'
Expand Down Expand Up @@ -46,13 +47,18 @@ export function isPrimitive(v: any) {
}

export function toFilePath(id: string, root: string): string {
let absolute = id.startsWith('/@fs/')
? id.slice(4)
: id.startsWith(root)
? id
: id.startsWith('/')
? resolve(root, id.slice(1))
: id
let absolute = (() => {
if (id.startsWith('/@fs/'))
return id.slice(4)
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
}
return id
})()

if (absolute.startsWith('//'))
absolute = absolute.slice(1)
Expand Down
41 changes: 22 additions & 19 deletions pnpm-lock.yaml

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

27 changes: 27 additions & 0 deletions test/core/test/file-path.test.ts
@@ -1,6 +1,9 @@
import { existsSync } from 'fs'
import { describe, expect, it, vi } from 'vitest'
import { isWindows, slash, toFilePath } from '../../../packages/vite-node/src/utils'

vi.mock('fs')

describe('toFilePath', () => {
// the following tests will work incorrectly on unix systems
if (isWindows) {
Expand Down Expand Up @@ -37,8 +40,10 @@ describe('toFilePath', () => {
const expected = '/path/to/project/node_modules/pkg/file.js'

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

expect(slash(filePath)).toEqual(expected)
})
Expand All @@ -49,8 +54,11 @@ describe('toFilePath', () => {
const expected = '/path/to/project/node_modules/pkg/file.js'

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

expect(slash(filePath)).toEqual(expected)
})

Expand All @@ -60,8 +68,10 @@ describe('toFilePath', () => {
const expected = '/root/node_modules/pkg/file.js'

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

expect(slash(filePath)).toEqual(expected)
})
Expand All @@ -72,8 +82,10 @@ describe('toFilePath', () => {
const expected = '/root/node_modules/pkg/file.js'

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

expect(slash(filePath)).toEqual(expected)
})
Expand All @@ -84,10 +96,25 @@ describe('toFilePath', () => {
const expected = '/root/path/to/file.js'

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

expect(slash(filePath)).toEqual(expected)
})

it('unix with sibling path', () => {
const root = '/path/to/first/package'
const id = '/path/to/second/package/file.js'

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

expect(slash(filePath)).toEqual(id)
})
}
})
1 change: 1 addition & 0 deletions test/path-res-target/.eslintignore
@@ -0,0 +1 @@
!dist/
1 change: 1 addition & 0 deletions test/path-res-target/.gitignore
@@ -0,0 +1 @@
!dist/
2 changes: 2 additions & 0 deletions test/path-res-target/README.md
@@ -0,0 +1,2 @@
This package is here just to be a sibling package to `path-resolution`. It
contains no tests of its own.
1 change: 1 addition & 0 deletions test/path-res-target/dist/index.d.ts
@@ -0,0 +1 @@
export function sub(a: number, b: number): number
3 changes: 3 additions & 0 deletions test/path-res-target/dist/index.js
@@ -0,0 +1,3 @@
export function sub(a, b) {
return a - b;
}
6 changes: 6 additions & 0 deletions test/path-res-target/package.json
@@ -0,0 +1,6 @@
{
"name": "@vitest/test-path-res-target",
"type": "module",
"private": true,
"main": "./dist/index.js"
}
15 changes: 15 additions & 0 deletions test/path-resolution/package.json
@@ -0,0 +1,15 @@
{
"name": "@vitest/test-path-resolution",
"type": "module",
"private": true,
"scripts": {
"test": "vitest",
"coverage": "vitest run --coverage"
},
"devDependencies": {
"@edge-runtime/vm": "1.1.0-beta.26",
"@vitest/test-path-res-target": "workspace:*",
"@vitest/web-worker": "workspace:*",
"vitest": "workspace:*"
}
}
17 changes: 17 additions & 0 deletions test/path-resolution/test/sister-package.test.ts
@@ -0,0 +1,17 @@
import { expect, it, vitest } from 'vitest'

import { sub } from '@vitest/test-path-res-target'

vitest.mock('@vitest/test-path-res-target')

it('should be mocked', () => {
expect(sub).toHaveProperty('mock')
expect(sub(5, 3)).toBeUndefined()
})

it('should import actual', async () => {
const { sub } = await vitest.importActual('@vitest/test-path-res-target')

expect(sub).not.toHaveProperty('mock')
expect(sub(5, 3)).toBe(2)
})
7 changes: 7 additions & 0 deletions test/path-resolution/vitest.config.ts
@@ -0,0 +1,7 @@
import { defineConfig } from 'vite'

export default defineConfig({
test: {
include: ['test/*.test.ts'],
},
})

0 comments on commit cc72d79

Please sign in to comment.