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

Allow keyFields and keyArgs functions to return false #7900

Merged
merged 4 commits into from
Mar 25, 2021

Conversation

CarsonF
Copy link
Contributor

@CarsonF CarsonF commented Mar 25, 2021

Previously the function was not allowed to return false even though false is supported

@apollo-cla
Copy link

@CarsonF: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@CarsonF
Copy link
Contributor Author

CarsonF commented Mar 25, 2021

Dang I thought this could be a quick fix. All I know is

keyArgs: () => []

results in "Foo:{}"

But

keyArgs: () => false

results in "Foo".
This works fine and is needed in my code because I need "Foo" and "Foo:{}" to be consolidated into a single stored field.

Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

@CarsonF Your use case makes sense, and you're right that we already handle falsy returns from both keyFields and keyArgs functions, so this additional return type is safe. Thanks for catching this!

Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

On second thought, if you could look into those test failures, that'd be great. I'm also happy to jump in if you run out of time.

@CarsonF
Copy link
Contributor Author

CarsonF commented Mar 25, 2021

Thanks @benjamn! It would be lovely if you could jump in. I didn't study the implementation and am only ignoring the type errors in my codebase.
I'm guessing that string | undefined is actually ok as string | false | undefined since we have the falsey checks instead of nullish checks, right?

@benjamn benjamn self-assigned this Mar 25, 2021
@benjamn benjamn changed the title Fix key specifier false type Allow keyFields and keyArgs functions to return false Mar 25, 2021
CarsonF and others added 2 commits March 25, 2021 18:33
Previously these functions were not allowed to return false even though
false is supported.
@CarsonF
Copy link
Contributor Author

CarsonF commented Mar 25, 2021

Beautiful!

Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

Thanks again for getting this started @CarsonF!

@benjamn benjamn merged commit a896f34 into apollographql:main Mar 25, 2021
@CarsonF CarsonF deleted the patch-1 branch April 11, 2021 16:15
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 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.

None yet

3 participants