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
Improve TypeScript type-safety of
cache.modify
#10895Improve TypeScript type-safety of
cache.modify
#10895Changes from 7 commits
0faf222
5f84a0b
4568407
9f7d536
1cfb03b
014f17d
6973cd4
4c12626
18e8bf9
fc50711
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.
I am afraid tests are excluded from typechecking. I intentionally created a typechecking error and
npm run build
passedAm I missing something?
If not, then perhaps we should start also typechecking tests to catch errors there. It would probably be safest to do in a separate PR. I'm just bringing it up here as I noticed a possible improvement (or a lack of understanding on my part)
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.
They are from building, but they are checked when running the tests - also in CI.
See this CI run: https://app.circleci.com/pipelines/github/apollographql/apollo-client/20241/workflows/ece99794-faac-4ed0-b938-f8fe5e2b697c/jobs/102866
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 see. Thanks for the info. I missed that typechecking happens during tests.
n cases like this one, I usually see 2 TS projects: one for the source code, one for the tests. The build typechecking (that happens in
npm run build
) can only compile the code project, while there could be a separate npm script that typechecks both projects. This way the typechecking would be easier to discover. Just some food for thought from an external contributor 🙂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.
@Gelio I'm late to the party, but figured I'd offer you (and others that stumble upon this comment) an explanation as to why you're seeing this.
We deliberately disabled strict type checking in development (#10268) because we wanted the ability to comment out code when debugging issues against failing tests while allowing our tests to run with broken types. Sometimes the easiest way to figure out why something is failing is to comment out specific parts of code to figure out what line of code is causing the issue. Debugging like this in some cases cascaded all over the place (I remember one of these to spot check a specific line of code and I ended up having to comment 50-60 lines of code just to get the test to run).
Instead, we allow TypeScript errors, but have it just show a warning at the very top of the tests. This allows us to see there are type issues, but our tests will still run. As @phryneas said, we have strict type checking when running in CI, so it will be caught in PRs and such.
Hope this helps!
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.
@jerelmiller Thanks for a thorough explanation!
I understand the problem you were trying to solve. I would also be frustrated at the tests not attempting to run because there are some unused symbols in the code
A solution I see more commonly used in the wild is fully separating the typechecking from testing. I don't see much of the reason for
jest
(orts-jest
) to do both. As a fellow engineer, I would much rather have a dedicated npm script for typechecking, and a separate one for just running the tests (without checking the types). That would be less of a surprise than the current state of affairs 🙂Maybe you would also get faster feedback this way, since
jest
would only strip the types and not have to compile the project first. This opens up possibilities of using a different jest transpiler, like @swc/jest, which could be faster at transpiling TS than tscI thought I would share this perspective of a first-time contributor.