Restrict ObjectCanon prototypes to {Array,Object}.prototype and null. #8260
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benjamn
commented
May 18, 2021
Comment on lines
-129
to
+130
| case "[object Object]": { | ||
| case null: | ||
| case Object.prototype: { |
There was a problem hiding this comment.
The case "[object Object]" from before would have canonized most user-defined classes (the ones that don't redefine Symbol.toStringTag), which feels dangerous to me, because not every class can be canonized (canonization requires the ability to copy the object safely, for example). A more complete solution will allow defining appropriate canonization logic for specific object types/prototypes, in addition to the defaults.
As promised in #8222 (comment), a more general implementation of ObjectCanon is coming, so I think we should restrict the functionality of the current implementation to match what we have planned. First, I want to reserve the isCanonical method to return true for anything that doesn't need to be (further) canonized, including primitive values. To that end, the isKnown method now returns true only for *objects* previously returned by canon.admit. Second, the future implemenation will allow full customization of canonization by object prototype, but will only canonize arrays and plain objects by default. To that end, I've resricted ObjectCanon canonization to objects whose prototypes are Array.prototype, Object.prototype, or null.
benjamn
force-pushed
the
restrict-ObjectCanon-prototypes-for-now
branch
from
ce247bf to
f1cda57
Compare
hwillson
approved these changes
May 18, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
As promised in #8222 (comment), a more general implementation of
ObjectCanonis coming (after Apollo Client v3.4), so I think it would be wise to restrict the functionality of the current implementation to match what I have planned.First, I want to reserve the
isCanonicalmethod to returntruefor anything that doesn't need to be (further) canonized, including primitive values. To that end, theisKnownmethod now returnstrueonly for objects previously returned bycanon.admit.Second, the future implementation will allow full customization of canonization by object
prototype, but will only canonize arrays and plain objects by default. To that end, I've restrictedObjectCanoncanonization to objects whose prototypes areArray.prototype,Object.prototype, ornull.