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

parse function resolves "extends": ".." incorrectly #149

Closed
sapphi-red opened this issue Dec 19, 2023 · 7 comments · Fixed by #150
Closed

parse function resolves "extends": ".." incorrectly #149

sapphi-red opened this issue Dec 19, 2023 · 7 comments · Fixed by #150

Comments

@sapphi-red
Copy link

Description

When the tsconfig.json has "extends": ".." and that pointed directory contains package.json. parse function resolves that to the main field. But TypeScript and parseNative function resolves that to ../tsconfig.json.

Reproduction

https://stackblitz.com/edit/node-k9hb7p?file=index.js

Running tsc -p foo --showConfig returns the resolved config by TypeScript.

Additional Information

Output of npx envinfo --system --binaries --browsers --npmPackages tsconfck (Note: the command in the template (npx envinfo --system --binaries --browsers --npmPackages "{tsconfck}") didn't include the tsconfck version number)

  System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.18.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.4.2 - /usr/local/bin/npm
    pnpm: 8.10.5 - /usr/local/bin/pnpm
  npmPackages:
    tsconfck: ^3.0.0 => 3.0.0
@dominikg
Copy link
Owner

obvious workaround: use ../tsconfig.json instead of ..

@dominikg
Copy link
Owner

dominikg commented Dec 19, 2023

fortunately this is used rarely, less than 200 json files on github with this value https://github.com/search?q=%2F%22extends%22%5Cs*%3A%5Cs*%22%5C.%5C.%22%2F+path%3A*.json&type=code&p=5 and not all of them with parents that have tsconfig.json and package.json.

Is there any documentation in typescript on priority with ambiguous references? They have some obscure rules, this issue reminded me of evanw/esbuild#3247 also... so what would happen if the package.json also had a tsconfig field?

can't wait until they decide to move to flat config like eslint...

@sapphi-red
Copy link
Author

Is there any documentation in typescript on priority with ambiguous references? They have some obscure rules, this issue reminded me of evanw/esbuild#3247 also... so what would happen if the package.json also had a tsconfig field?

I don't know if there are. Apparently, tsconfig field has a higher priority.
https://stackblitz.com/edit/node-pefgtv?file=tsconfig2.json

@dominikg
Copy link
Owner

@JounQin

I had a look at typescripts implementation and it feels a bit like this works on accident rather than on purpose.

https://github.com/microsoft/TypeScript/blob/d027e9619fb8ca964df3885a536a67b5f813738b/src/compiler/commandLineParser.ts#L3325

For relative paths (starting with ./ or ../) they check the file system directly, only because they omitted the literal '..' from the list of checks it moves on to being resolved via node, after appending tsconfig.json. Relative paths only get .json appended if it's missing.

Which means it would fail if you had ../.. instead of .. because it would look for ../...json

Is there a specific reason you are using .. over the more explicit ../tsconfig.json ?

@dominikg
Copy link
Owner

It doesn't really help that the reference for tsconfig extends only mentiones "it may use node style resolution" https://www.typescriptlang.org/tsconfig#extends but does not go into detail how extensions or the whole filename can be omitted, or how it also appears to work if a package has a tsconfig top level key.

@JounQin
Copy link

JounQin commented Dec 28, 2023

Is there a specific reason you are using .. over the more explicit ../tsconfig.json ?

  1. It's much cleaner to me
  2. This is just how typescript works, we should align the behavior although it could be supported accidentally or even a bug feature

@dominikg
Copy link
Owner

to make tsconfck extends resolve work exactly like typescripts, it would have to copy the whole code they use, which is a lot (follow the link i provided above). To stay fast and lean, i think it is ok to not implement edge cases that are rarely used/undocumented.

That being said, the current resolve extends implementation favors node resolve over fs paths which can be expensive (createRequire), so doing fs checks first can be added with a reasonable amount of code.

So in

function resolveExtends(extended, from) {

it could check for an fs path and then skip createRequire entirely. At that point a special case handling for .. could be added. This can be done in a few lines of code.

alvis added a commit to alvis/presetter that referenced this issue Jan 25, 2024
NOTE: This is a fix to an edge case of some third party tools don't honor . as a
valid path. See dominikg/tsconfck#149
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 a pull request may close this issue.

3 participants