dominikg / tsconfck

A utility to find and parse tsconfig files without depending on typescript
Other
293 stars 14 forks source link

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

Closed sapphi-red closed 10 months ago

sapphi-red commented 10 months ago

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 commented 10 months ago

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

dominikg commented 10 months ago

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 https://github.com/evanw/esbuild/issues/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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 https://github.com/dominikg/tsconfck/blob/ecd5d375077e4df1482772918e3ff9576175c73c/packages/tsconfck/src/parse.js#L209

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.