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

Add resolved typeRoots into the discarded includes to maintain global types. #22

Closed

Conversation

kylorhall
Copy link

@kylorhall kylorhall commented Nov 30, 2021

This is an idea to fix #20.

Instead of discarding all includes, which discards required, global .d.ts type files, can we use the typeRoots a config may have setup instead?

For me, this basically results in:

-includes: [].
+includes: [
+  '/Users/…/repo/types',
+  '/Users/…/repo/node_modules/@types',
+],

I don't think this quite hits the mark, however:

  • Does everyone use typeRoots in this manner? I do not know.
  • Likely will not work for a monorepo with nested tsconfig as files are resolved to the root. Maybe resolveFromRoot is not even required..
  • Often typeRoots will import a fair bit, this could still include a large % of your codebase. My typeRoots includes node_modules/@types.

Ultimately this may defeat the purpose #18 set out to resolve…but I guess this could be documented. Alternatively, I might suggest you could pass --typeRoots ./types/ or something via CLI, but I wasn't so comfortable implementing that with the remainingArgsToForward to be frank.

This is an idea to fix gustavopch#20.

Instead of discarding all includes, which seems to discard required global `.d.ts` files, can we use the `typeRoots` a config may have setup instead?

I don't think this quite hits the mark however:
 - Does everyone use `typeRoots` in this manner?  I do not know.
 - Likely will not work for a monorepo with nested tsconfig as files are resolved to the root.
 - Often `typeRoots` will import a fair bit, this could still include a large % of your codebase.  My `typeRoots` includes `node_modules/@types`–this may defeat the purpose gustavopch#18 set out to resolve.
@gustavopch
Copy link
Owner

gustavopch commented Nov 30, 2021

@kylorhall As you said, it won't work for all use cases. Even if it's not a monorepo, some people (like me) use @types folder instead of types.

@gustavopch
Copy link
Owner

@kylorhall Also, I think typeRoots shouldn't even need to be duplicated into include to work. I usually do this:

{
  "compilerOptions": {
    "typeRoots": ["@types", "node_modules/@types"]
  },
  "include": ["src"]
}

That's enough for TypeScript to read the @types folder when type-checking — no need to also add that folder to include. Doesn't that work for you?

@kylorhall
Copy link
Author

kylorhall commented Nov 30, 2021

Even if it won't work for all use-cases … @types

Any named typeRoots folder(s) should work: ./typings, ./@types, etc; just pulls from what is in compilerOptions.typeRoots.


That's enough for TypeScript to read the @types folder when type-checking — no need to also add that folder to include. Doesn't that work for you?

🤔 No, should it?

That's the core problem defined in #20, unless I/others in that issue, are missing something…I always thought this was a common tsconfig: include: ['./src', './types']. So then the change in #18 to files: ['file.ts'], include: [] does not work as global (local) types will be forever missing.

compilerOptions.typeRoots doesn't seem to inject those global types whatsoever for tsc at all as include: ['./types'] appears required. Maybe I/we are just doing something wrong?


Maybe this minimal repo will help clear things up: https://github.com/kylorhall/tsc-files-types-example

Follow steps to replicate, a commit to files.ts will fail with tsc-files@1.1.3. But, with a yarn link tsc-files, this PR passes on the above repo, so maybe a starting point to try out a few more scenarios?

@gustavopch
Copy link
Owner

@kylorhall Thanks for the repro! Now please try to move types/global.d.ts to types/global/index.d.ts and run npx tsc-files src/file.ts. It should understand the global types and pass. Then add some type error like const x: string = 1 and run it again. It should detect the type error and fail. Try it and tell me if it works. If it does, then this PR won't be needed. Instead, we'll just need to advise users on how to use typeRoots correctly.

kylorhall added a commit to kylorhall/tsc-files-types-example that referenced this pull request Dec 4, 2021
…est scenario.

The resolution just appears we're using typeRoots incorrectly!

See comment: gustavopch/tsc-files#22 (comment)
@kylorhall
Copy link
Author

kylorhall commented Dec 7, 2021

Try it and tell me if it works.

Sorry it took a while. It definitely worked in that example repo: kylorhall/tsc-files-types-example#1, but it took a long time to figure out why it didn't happen in the full org codebase…

I believe I've gotten to the bottom of this and it's a combination of several misconceptions…was a bit more complex than the provided repo. Even reading issues like microsoft/TypeScript#22217 and solutions like https://stackoverflow.com/a/45887328 (which suggested this bad practice), I still don't have an answer, but I think I've got it. I'll have someone else on my team confirm, but it does appear to just be a bad TypeScript implementation and perhaps a TypeScript documentation issue.

Our issue was the combination of these three settings:

{
  "compilerOptions": {
   "types": ["jest", "node"],
    "typeRoots": ["./types/", "./node_modules/@types"],
  },
  "include": ["./src/", "./types/"],
}

My steps to resolve:

  1. 🤦 Remove types entirely. This meant only jest and node were auto-included from types or node_modules/@types. I didn't even look at it, it's essentially the same as include, except for types.
  2. Remove ./types from my include paths, so just include: ['./src'].
  3. I rebuilt a few things in ./types because they were not definition files…but that's irrelevant to this.
{
  "compilerOptions": {
-   "types": ["jest", "node"],
    "typeRoots": ["./types/", "./node_modules/@types"],
  },
- "include": ["./src/", "./types/"],
+ "include": ["./src/"],
}

With that, I wholeheartedly agree that doing includes: compilerOptions.typeRoots is completely incorrect…it might work in a few nuanced scenarios, but then you'd be explicitly including node_modules/@types for most people, it defeats the purpose. Thanks for the direction!


Closing, will comment in the issue with a link back to here.

For me on a single file:

  • 🎉 v1.1.3 takes roughly ~6s to do a git commit -am "…" – so thank you!
  • 🐌 v1.1.2 takes roughly ~17.5s to do a git commit -am "…"

@kylorhall kylorhall closed this Dec 7, 2021
@kylorhall kylorhall deleted the kylorhall/include-type-roots branch December 7, 2021 09:54
@gustavopch
Copy link
Owner

@kylorhall Glad to know! Thanks for the detailed explanation.

@gustavopch
Copy link
Owner

I'm locking so the discussion is centralized in #20.

Repository owner locked and limited conversation to collaborators Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Current version incorrectly analyzes @types/node
2 participants