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

QueryManager, fix "invalidate" -> "invalidated" typo i think. #4041

Merged
merged 4 commits into from
Jan 29, 2019

Conversation

quazzie
Copy link
Contributor

@quazzie quazzie commented Oct 24, 2018

Was hunting for another bug and came across this.
I didn't find any other reference to "invalidate" property only "invalidated".

Someone with more knowledge of the code maybe knows better.

Was hunting for another bug and came across this. 
I think it is suppose to be "invalidated"
@hwillson
Copy link
Member

hwillson commented Jan 2, 2019

Thanks for pointing this out @quazzie! This is a bug, although it's benign since the misuse of invalidate: false doesn't impact any functionality. That being said, this problem could have been avoided if the setQuery function signature was updated to not use a TS any:

private setQuery(queryId: string, updater: (prev: QueryInfo) => any) {

Your PR is correct, but I'd like to leave this open a bit longer, to get a proper TS fix in place. Thanks!

@hwillson hwillson self-assigned this Jan 3, 2019
@danilobuerger
Copy link
Contributor

@hwillson
I don’t think it is possible to tighten the typing of setQuery to catch that error. First thought would be to use Partial<QueryInfo>. However, there is no excess checking on the object literals. So it still wouldn't catch a typo. I would suggest modifying setQuery to:

private setQuery(queryId: string, updater: (prev: QueryInfo) => QueryInfo)

and having callers merge the prev into the result instead of doing that in setQuery.
If you agree, I would create a PR for this.

@hwillson
Copy link
Member

@danilobuerger Hmmm - setQuery is called quite a bit in the QueryManager, so moving shared functionality out of it might be a bit of a headache (especially if people forget to handle the prev merging themselves in the future). This isn't a big issue though, so we could just fix the typo and move on. Thanks!

@danilobuerger
Copy link
Contributor

danilobuerger commented Jan 22, 2019

@hwillson They couldn't forget it because of the return type being the full QueryInfo. So either they specify the whole QueryInfo in which case they don't need prev anyway since that gets overwritten, or they only specify it partially and that will throw a type error. So there is no way to forget it. But if we leave it like it is now, there will always be the chance of typos getting in.

@hwillson
Copy link
Member

Thanks @danilobuerger - we'll definitely review a PR for this!

@danilobuerger
Copy link
Contributor

Actually, what I said is not working either, as this suffers from the same lack of excess checking. I however found a solution with Pick that keeps setQuery working as is. Will PR now.

danilobuerger added a commit to danilobuerger/apollo-client that referenced this pull request Jan 24, 2019
@danilobuerger danilobuerger self-assigned this Jan 24, 2019
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @quazzie!

@hwillson hwillson merged commit 5588cf9 into apollographql:master Jan 29, 2019
hwillson pushed a commit that referenced this pull request Jan 29, 2019
* Properly type setQuery and fix now typed callers

Fixes #4041

* Changelog update
@quazzie quazzie deleted the patch-2 branch January 29, 2019 20:22
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants