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

[node] Don't resolve "typescript" from the dist dir #4655

Merged
merged 1 commit into from
Jun 14, 2020

Conversation

TooTallNate
Copy link
Member

@TooTallNate TooTallNate commented Jun 14, 2020

On Node 10, the require.resolve() with "paths" does not return the
proper value relative to the node_modules directory. To wit:

$ node -v
v10.16.3

$ node -p "require.resolve('typescript', { paths: [process.cwd()] })"
/Users/nrajlich/Code/vercel/vercel/packages/now-node/dist/typescript.js

$ node -v
v14.4.0

$ node -p "require.resolve('typescript', { paths: [process.cwd()] })"
/Users/nrajlich/Code/vercel/vercel/node_modules/typescript/lib/typescript.js

(Note: cwd when running these commands is the dist dir of @vercel/node)

So the solution is to just let require.resolve() throw an error so the
default string "typescript" is used instead of a resolved absolute path.

On Node 10, the `require.resolve()` with "paths" does not return the
proper value relative to the `node_modules` directory. To wit:

```
$ node -v
v10.16.3

$ node -p "require.resolve('typescript', { paths: [process.cwd()] })"
/Users/nrajlich/Code/vercel/vercel/packages/now-node/dist/typescript.js

$ node -v
v14.4.0

$ node -p "require.resolve('typescript', { paths: [process.cwd()] })"
/Users/nrajlich/Code/vercel/vercel/node_modules/typescript/lib/typescript.js
```

So the solution is to just let `require.resolve()` throw an error so the
default string "typescript" is used instead of a resolved absolute path.
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine, but also very strange that Node 10 would resolve differently and that none of our Node 10 tests would catch this?

@TooTallNate
Copy link
Member Author

This is the specific fix for the issue which landed in Node 12.0.0 release: nodejs/node#23683.

@kodiakhq kodiakhq bot merged commit 5021a71 into master Jun 14, 2020
@kodiakhq kodiakhq bot deleted the fix/ts-node-ts.sys.fileExists-error branch June 14, 2020 20:47
TooTallNate added a commit that referenced this pull request Jun 14, 2020
…@vercel/node`

Adds a test case for #4655 (it got auto-merged too quickly).
kodiakhq bot pushed a commit that referenced this pull request Jun 14, 2020
…@vercel/node` (#4656)

Adds a test case for #4655 (it got auto-merged too quickly).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants