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

Make keyArgs tolerant of optional arguments. #7109

Merged
merged 2 commits into from
Oct 2, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Oct 1, 2020

As suggested by @tadhglewis in #6973.

The keyArgs: ["someArg", "anotherArg"] configuration for field policies should be able to include arguments that are optional, while still specifying an ordering of all possible key arguments for serialization purposes.

When an optional argument is not provided, it will simply not be included in the serialized storeFieldName suffix, and no exception will be thrown.

We cannot safely do the same thing for keyFields: [...] in type policies, since a consistent set of primary key fields (for a given __typename) is required to (re)identify objects over time.

The keyArgs:[...] configuration for field policies should be able to
include arguments that are optional, while still specifying an ordering of
all possible key arguments for serialization purposes. When an optional
argument is not provided, it will simply not be included in the serialized
storeKeyName suffix, and no exception will be thrown.

Should address #6973.
if (hasOwn.call(response, responseName)) {
keyObj[prevKey = s] = response[responseName];
} else {
invariant(!strict, `Missing field '${responseName}' while computing key fields`);
Copy link
Member Author

Choose a reason for hiding this comment

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

We use this computeKeyObject helper function for both keyArgs and keyFields, but because strict is always false in the keyArgs case, we never throw an exception here, which is good because "while computing key fields" was a poor error message for a problem with keyArgs.

@benjamn benjamn requested review from hwillson and jcreighton October 1, 2020 21:13
}
} else {
const aliases = aliasMap && aliasMap.aliases;
const responseName = aliases && aliases[s] || s;
invariant(
hasOwn.call(response, responseName),
// TODO Make this appropriate for keyArgs as well
Copy link
Member Author

Choose a reason for hiding this comment

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

TODONE

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.

Looks great @benjamn - thanks!

@benjamn benjamn merged commit 8163dd9 into release-3.3 Oct 2, 2020
@benjamn benjamn deleted the 6973-make-keyArgs-tolerant-of-optional-args branch October 2, 2020 17:00
@robertsmit
Copy link

robertsmit commented Dec 4, 2020

I still have the issue in 3.3 with optional arguments not specified by the query, I still get the error:

Uncaught Invariant Violation: Missing field 'activeOn' while computing key fields

Were activeOn is specified in the typePolicy keyArgs but not specified and occuring in the query .

@benjamn
Copy link
Member Author

benjamn commented Dec 7, 2020

@robertsmit Are you absolutely sure you're working with keyArgs: ["activeOn", ...] (which goes in a field policy) rather than keyFields (which goes in a type policy)? Can you put together a small reproduction to demonstrate the problem, in a separate issue (but linking back to this PR)?

@robertsmit
Copy link

I still have the issue in 3.3 with optional arguments not specified by the query, I still get the error:

Uncaught Invariant Violation: Missing field 'activeOn' while computing key fields

Were activeOn is specified in the typePolicy keyArgs but not specified and occuring in the query .

Currently no problems any more... maybe I did used keyFIelds instead of keyArgs...

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 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