Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(typescript): Fix incorrect type generated for glob route parameters | Add type tests #6364
fix(typescript): Fix incorrect type generated for glob route parameters | Add type tests #6364
Changes from 13 commits
3330c73
09e462e
6c86058
73d5906
b98bba6
db82751
7ca4f78
fbd69a0
e671ab2
d870521
684f7c0
806d120
50a7671
1845e62
6fdeae5
88edc39
d860055
061bcc8
8706a05
e68fb7f
64758f6
c1d424f
9e3f586
37e1c79
7359f8c
b80f2d2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try adding the following lines and importing from
.ts
file. Does it help?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into it!
Yep I had to set composite, but also set incremental - because the base tsconfig contains
tsBuildInfo
- which needs one of these flags. I'm not 100% sure the impact this has though, but since its only for the tsd test environment should be ok, I thinkThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually had to add all of these 😢 - because of conflicting flags set in the original config. I suppose these settings only affect the build - which we're not in the case of type tests
Is the fact that the tsconfig in the tests drifting away from the project config an issue?
You're right about composite definitely being the problem... but if I check the config with tsc here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the file is included. I see the same config from inside
tsd-lite
. It is using TS compiler’s APIs to find and read config.The error we saw comes from TS compiler and it looks misleading. Here is an experiment:
tsconfig.json
to__typetests__
folder.yarn tsc -p src/__typetests__
. All works."compilerOptions": {"composite": true}
to the sametsconfig.json
and run the same command.tsc
this time."references": [{ "path": "../../" }]
to the sametsconfig.json
and run the command.All works again. Isn’t it that with
"composite": true
each directory withtsconfig.json
turns into a project? So in this case the compiler complains, because it seessrc
and__typetests__
as separate projects.By the way,
tsd-lite
wraps TS compilers’s APIs to typecheck and compare types. Iftsc
complains,tsd-lite
will complain in the same way. EDIT: Only thattsd-lite
compiles in memory and does not emit anything, so these extra emit options have no impact.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation Tom, makes sense.
The TS docs lack a little bit on
composite
andincremental
- but it certainly seems to behave the way you're describing. I'll close the issue on the jest runner side :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait. I just realised that
tsc
needs-b
flag to work with composites, not-p
. So error comes from something else. Hm.. Just tried to useyarn tsc -b src/__typetests__
with previous experiment. Same result.Back to your question,
tsd-lite
looks for atsconfig.json
nearest to a test file. If not found, it will use defaults of the compiler. If you delete__typetests__/tsconfig.json
,tsd-lite
will use config frompackages/router
. It gives the same error, adding{"composite": false}
topackages/router/tsconfig.json
makes it work.Very interesting case. I have to dig deeper into this to understand. In any case, since
tsc
throws the same error this must be a config issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can’t stop ;D I just looked at Jest repo where
tsd-lite
is used (with 1200+ assertions passing) and in__typetests__/tsconfig.json
we have"noUnusedLocals": false
,"noUnusedParameters": false
, etc. In a larger test suite these are useful. With time it becomes handy to have dedicatedtsconfig.json
for type tests, although it might look strange at first.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean to ask a much simpler question, why doesn't tsd-lite work with composite? Just for knowledge - I don't quite grasp what the composite flag is for (outside of a build-time optimisation). I wouldn't be surprised if it's just a misconfiguration on our side.
Thank you very much for looking into it - glad it's piqued your interest!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my - I think I spotted the problem! Look at the last exclude glob, in the output for the tsc config it ignores the test files 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I was looking at that glob. Might be the cause, but note that
routeParamsTypes.ts
is reported as not included, which is not a test file. Also you need those globs for build. So they have to stay.I think
tsd-lite
should work with composites, but with__typetests__/tsconfig.json
it needsreferences
. Just liketsc
in my experiment, which I described above. At the same time,composite
is useful for building libraries, there is not much use of it in testing. Perhaps there is a use case, but I think"composite": false
is better solution in your case.