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

Avoid letting defaultDataIdFromObject normalize objects with nullish ids #9862

Merged
merged 2 commits into from
Sep 21, 2022

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jun 27, 2022

Should fix issue #9814, by not allowing objects with id: null or id: void 0 fields to be normalized.

This change mostly affects applications that have not configured keyFields or a custom dataIdFromObject function when creating the InMemoryCache, though I suppose the same confusion could happen when keyFields: ["id"] is explicitly configured and the object.id happens to be null or undefined.

I'm borrowing the "nullish" terminology introduced by the Nullish Coalescing Operator ??, which treats only null and undefined as falsy (not "", 0, or false).

This distinction allows objects with id: "" and id: 0 to be normalized, even though "" and 0 are falsy values. While "" and 0 may be unusual ID values, it's conceivable that an incrementing counter could start at 0, or a string ID could sometimes be the empty string.

You could perhaps argue that objects with id: false also should not be normalized, since false a constant value that's uncommon for IDs, but null and undefined show up due to ordinary GraphQL error behavior, so id: false suggests some actual intention to return a constant value. My mind isn't fully made up about this, but I'm erring on the side of preserving existing behavior.

@benjamn benjamn added this to the Release 3.7 milestone Jun 27, 2022
@benjamn benjamn self-assigned this Jun 27, 2022
@benjamn benjamn requested review from alessbell and hwillson June 27, 2022 18:26
@benjamn benjamn linked an issue Jun 27, 2022 that may be closed by this pull request
Copy link
Contributor

@alessbell alessbell left a comment

Choose a reason for hiding this comment

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

Seems reasonable to treat both null and undefined as special cases, given that they can appear due to ordinary GraphQL error behavior as you noted. Should we update any docs, in this PR or a follow-on?

@hwillson hwillson removed their request for review July 20, 2022 11:08
@alessbell alessbell merged commit b091220 into release-3.7 Sep 21, 2022
@alessbell alessbell deleted the issue-9814-avoid-normalizing-nullish-ids branch September 21, 2022 16:31
@gurianov-p
Copy link

I can see your point about nullish values, but isn’t it technically a breaking change? I think there are perfectly valid use cases where null as ID is appropriate and there are plenty of Apollo users whose code will break once they update to v3.7. For example, we are using null as ID for anonymous users' session data (as opposed to logged in users). For the sake of preserving existing behavior, this change should’ve been introduced in the next major release, not minor.

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

null ID should not be considered cacheable keys
3 participants