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.

Sign up for GitHub

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

Jump to bottom

importActual mangles file paths when importing sister packages in Lerna repos #1864

Closed
6 tasks done
simon-abbott opened this issue Aug 16, 2022 · 2 comments · Fixed by #1868
Closed
6 tasks done

importActual mangles file paths when importing sister packages in Lerna repos #1864

simon-abbott opened this issue Aug 16, 2022 · 2 comments · Fixed by #1868

Comments

@simon-abbott
Copy link
Contributor

simon-abbott commented Aug 16, 2022 •
edited
Loading

Describe the bug

Calling importActual results in vitest trying (and failing) to resolve an incredible mangled file path if the imported package is a sister package (not having the same root directory) and its main file is in a dist folder.

It seems the root cause of this issue is that toFilePath doesn't actually respect absolute paths, and instead tries to prefix them with the cwd. If this mangled path contains /dist/, it is then fed to isValidNodeImport() (src), which then tries to read from that path (which doesn't exist) and throws.

Reproduction

https://github.com/simon-abbott/vitest-lerna-mre -- just run npm i and npm run test. Also try modifying packages/one so that index.js isn't in dist/ and the error goes away.

System Info

System:
    OS: macOS 12.4
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 5.27 GB / 32.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.7.0 - ~/.nvm/versions/node/v18.7.0/bin/node
    npm: 8.15.0 - ~/.nvm/versions/node/v18.7.0/bin/npm
    Watchman: 2022.07.04.00 - /usr/local/bin/watchman
  Browsers:
    Brave Browser: 90.1.24.86
    Chrome: 104.0.5112.79
    Firefox: 103.0.2
    Firefox Developer Edition: 104.0
    Safari: 15.5

Used Package Manager

npm

Validations
@simon-abbott simon-abbott changed the title importActual mangles file paths in Lerna repos importActual mangles file paths when importing sister packages in Lerna repos Aug 16, 2022
@simon-abbott
Copy link
Contributor Author

simon-abbott commented Aug 16, 2022 •
edited
Loading

This also raises the question of why /dist/ is being special-cased in the first place. If /dist/ files were treated the same as bare files (or /lib/, or /out/, etc.), then this bug wouldn't have happened.

vitest/packages/vite-node/src/externalize.ts

Lines 83 to 84 in 57c2367

const isDist = id.includes('/dist/')
if ((isNodeModule || isDist) && await isValidNodeImport(id))

simon-abbott added a commit to simon-abbott/vitest that referenced this issue Aug 17, 2022
@simon-abbott
fix: don't use resolved paths if they don't actually exist
ffc9179 
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 vitest-dev#1864.
simon-abbott added a commit to simon-abbott/vitest that referenced this issue Aug 17, 2022
@simon-abbott
fix: handle case where path starts with root but isn't contained inside
2e9b335 
Previously `getFsPath` would just replace the first occurrence of the
root path with the empty string (`path.replace(this.root, '')`). This
lead to incorrect behavior:

```js
const path = '/some/path/to/a/package-somewhere/index.js'
const root = '/some/path/to/a/package'
const result = path.replace(root, '')

// result _should_ be '/some/path/to/a/package-somewhere/index.js', but
// instead it's '-somewhere/index.js'
```

This fixes a bug I found while trying to write a test case for vitest-dev#1864.
@simon-abbott simon-abbott mentioned this issue Aug 17, 2022
Merged
simon-abbott added a commit to simon-abbott/vitest that referenced this issue Aug 17, 2022
@simon-abbott
fix: don't use resolved paths if they don't actually exist
cc72d79 
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 vitest-dev#1864.
simon-abbott added a commit to simon-abbott/vitest that referenced this issue Aug 17, 2022
@simon-abbott
fix: handle case where path starts with root but isn't contained inside
1efdd2d 
Previously `getFsPath` would just replace the first occurrence of the
root path with the empty string (`path.replace(this.root, '')`). This
lead to incorrect behavior:

```js
const path = '/some/path/to/a/package-somewhere/index.js'
const root = '/some/path/to/a/package'
const result = path.replace(root, '')

// result _should_ be '/some/path/to/a/package-somewhere/index.js', but
// instead it's '-somewhere/index.js'
```

This fixes a bug I found while trying to write a test case for vitest-dev#1864.
simon-abbott added a commit to simon-abbott/vitest that referenced this issue Aug 23, 2022
@simon-abbott
fix: don't use resolved paths if they don't actually exist
03ca410 
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 vitest-dev#1864.
simon-abbott added a commit to simon-abbott/vitest that referenced this issue Aug 23, 2022
@simon-abbott
fix: handle case where path starts with root but isn't contained inside
f60242d 
Previously `getFsPath` would just replace the first occurrence of the
root path with the empty string (`path.replace(this.root, '')`). This
lead to incorrect behavior:

```js
const path = '/some/path/to/a/package-somewhere/index.js'
const root = '/some/path/to/a/package'
const result = path.replace(root, '')

// result _should_ be '/some/path/to/a/package-somewhere/index.js', but
// instead it's '-somewhere/index.js'
```

This fixes a bug I found while trying to write a test case for vitest-dev#1864.
@pitkes22
Copy link

Any update on this?

@sheremet-va sheremet-va mentioned this issue Oct 28, 2022
Closed
6 tasks
simon-abbott added a commit to simon-abbott/vitest that referenced this issue Oct 28, 2022
@simon-abbott
fix: don't use resolved paths if they don't actually exist
47dab87 
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 vitest-dev#1864.
simon-abbott added a commit to simon-abbott/vitest that referenced this issue Oct 28, 2022
@simon-abbott
fix: don't use resolved paths if they don't actually exist
b18f707 
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 vitest-dev#1864.
sheremet-va pushed a commit that referenced this issue Oct 31, 2022
@simon-abbott
fix: don't use resolved paths if they don't actually exist (#1868)
6c3d708 
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.
@github-actions github-actions bot locked and limited conversation to collaborators Jun 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: importActual path resolution for non-child paths (fix #1864) simon-abbott/vitest
2 participants
@pitkes22 @simon-abbott