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

Fix TSConfig extends pointing to node module #4772

Merged
merged 1 commit into from
May 2, 2023
Merged

Conversation

jridgewell
Copy link
Contributor

@jridgewell jridgewell commented May 2, 2023

In #4754, I screwed up the resolution logic for configs that reference another config inside a node module. In it, I incorrectly joined /tsconfig.json before performing any lookup, because I thought the joining was happening in the combinePaths in https://github.com/microsoft/TypeScript/blob/611a912d/src/compiler/commandLineParser.ts#L3315. But, that combine has no real affect, it immediately gets stripped in a call to getDirectoryPath in https://github.com/microsoft/TypeScript/blob/611a912d/src/compiler/moduleNameResolver.ts#LL1703C93-L1703.

Instead, it performs a basic node_module lookup using the input module name, and even tries to load the file referenced at that location (eg, it's a extends: 'foo/tsconfig.json'). If that fails, then it combines with an implied /tsconfig the same way a /index is appended when doing require('./directory').

Fixes WEB-974

@jridgewell jridgewell requested a review from a team as a code owner May 2, 2023 03:26
@vercel
Copy link

vercel bot commented May 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

11 Ignored Deployments
Name Status Preview Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) May 2, 2023 3:26am
examples-cra-web ⬜️ Ignored (Inspect) May 2, 2023 3:26am
examples-designsystem-docs ⬜️ Ignored (Inspect) May 2, 2023 3:26am
examples-gatsby-web ⬜️ Ignored (Inspect) May 2, 2023 3:26am
examples-kitchensink-blog ⬜️ Ignored (Inspect) May 2, 2023 3:26am
examples-native-web ⬜️ Ignored (Inspect) May 2, 2023 3:26am
examples-nonmonorepo ⬜️ Ignored (Inspect) May 2, 2023 3:26am
examples-svelte-web ⬜️ Ignored (Inspect) May 2, 2023 3:26am
examples-tailwind-web ⬜️ Ignored (Inspect) May 2, 2023 3:26am
examples-vite-web ⬜️ Ignored (Inspect) May 2, 2023 3:26am
turbo-site ⬜️ Ignored (Inspect) May 2, 2023 3:26am

@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2023

✅ This change can build next-swc

@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2023

🟢 CI successful 🟢

Thanks

@@ -0,0 +1,3 @@
{
"extends": "tsconfig-mod/tsconfig.json"
Copy link
Member

Choose a reason for hiding this comment

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

I think the test case could be improve by also testing omitting the .json extension and naming the file different than tsconfig

Suggested change
"extends": "tsconfig-mod/tsconfig.json"
"extends": "tsconfig-mod/config"

@sokra sokra merged commit d8ae01e into main May 2, 2023
@sokra sokra deleted the jrl-tsconfig-extends-2 branch May 2, 2023 04:56
kodiakhq bot pushed a commit to vercel/next.js that referenced this pull request May 2, 2023
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
In #4754, I screwed up the resolution logic for configs that reference
another config inside a node module. In it, I incorrectly joined
`/tsconfig.json` before performing any lookup, because I thought the
joining was happening in the `combinePaths` in
https://github.com/microsoft/TypeScript/blob/611a912d/src/compiler/commandLineParser.ts#L3315.
But, that combine has no real affect, it immediately gets stripped in a
call to `getDirectoryPath` in
https://github.com/microsoft/TypeScript/blob/611a912d/src/compiler/moduleNameResolver.ts#LL1703C93-L1703.

Instead, it performs a basic `node_module` lookup using the input module
name, and even tries to load the file referenced at that location (eg,
it's a `extends: 'foo/tsconfig.json'`). If that fails, then it combines
with an implied `/tsconfig` the same way a `/index` is appended when
doing `require('./directory')`.

Fixes WEB-974
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
In #4754, I screwed up the resolution logic for configs that reference
another config inside a node module. In it, I incorrectly joined
`/tsconfig.json` before performing any lookup, because I thought the
joining was happening in the `combinePaths` in
https://github.com/microsoft/TypeScript/blob/611a912d/src/compiler/commandLineParser.ts#L3315.
But, that combine has no real affect, it immediately gets stripped in a
call to `getDirectoryPath` in
https://github.com/microsoft/TypeScript/blob/611a912d/src/compiler/moduleNameResolver.ts#LL1703C93-L1703.

Instead, it performs a basic `node_module` lookup using the input module
name, and even tries to load the file referenced at that location (eg,
it's a `extends: 'foo/tsconfig.json'`). If that fails, then it combines
with an implied `/tsconfig` the same way a `/index` is appended when
doing `require('./directory')`.

Fixes WEB-974
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
In #4754, I screwed up the resolution logic for configs that reference
another config inside a node module. In it, I incorrectly joined
`/tsconfig.json` before performing any lookup, because I thought the
joining was happening in the `combinePaths` in
https://github.com/microsoft/TypeScript/blob/611a912d/src/compiler/commandLineParser.ts#L3315.
But, that combine has no real affect, it immediately gets stripped in a
call to `getDirectoryPath` in
https://github.com/microsoft/TypeScript/blob/611a912d/src/compiler/moduleNameResolver.ts#LL1703C93-L1703.

Instead, it performs a basic `node_module` lookup using the input module
name, and even tries to load the file referenced at that location (eg,
it's a `extends: 'foo/tsconfig.json'`). If that fails, then it combines
with an implied `/tsconfig` the same way a `/index` is appended when
doing `require('./directory')`.

Fixes WEB-974
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants