Remove dependency on fast-json-stable-stringify
#8222
Merged
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
> Note: I am expecting tests to fail for this commit, demonstrating the importance of using a stable serialization strategy for field arguments. Although fast-json-stable-stringify has done its job well, it only provides a "main" field in its package.json file, pointing to a CommonJS entry point, and does not appear to export any ECMAScript modules. Thanks to our conversion/abandonment of fast-json-stable-stringify and other CommonJS-only npm dependencies (zen-observable in #5961 and graphql-tag in #6074), it should now (after this PR is merged) be possible to load @apollo/client/core from an ESM-aware CDN like jsdelivr.net or jspm.io: <script type=module src="https://cdn.jsdelivr.net/npm/@apollo/client@beta/core/+esm"> </script> If you put that script tag in an HTML file, or inject it into the DOM of any webpage, you will currently see this error: Uncaught SyntaxError: The requested module '/npm/fast-json-stable-stringify@2.1.0/+esm' does not provide an export named 'default' This list of errors used to be longer, but now the only package left is fast-json-stable-stringify. Note that we're loading @apollo/client/core@beta here, not @apollo/client@beta. The reason @apollo/client itself is not yet ESM-ready is that react and react-dom are the two remaining CommonJS-only dependencies, and @apollo/client currently/regrettably re-exports utilities from @apollo/client/react. If importing from @apollo/client/core is a burden or feels weird to you, please know that we are planning to make @apollo/client synonymous with @apollo/client/core in Apollo Client 4.0, along with making @apollo/client/react synonymous with the v3 API of @apollo/client, though of course those will be major breaking changes: #8190
benjamn
changed the title
fast-json-stable-stringify
This should fix the failing tests, though I'm planning to replace this default implementation with a more clever one in the next commit.
This reimplementation of the stable `stringify` function used by `getStoreKeyName` builds on the `ObjectCanon` introduced by my PR #7439, which guarantees canonical objects keys are always in sorted order.
brainkim
reviewed
May 14, 2021
benjamn
added a commit
that referenced
this pull request
May 14, 2021
This reset will happen whenever cache.gc() is called, which makes sense because it frees all memory associated with the replaced stringifyCanon, at the cost of temporarily slowing down canonicalStringify (but without any logical changes in the behavior/output of canonicalStringify).
benjamn
force-pushed
the
stop-depending-on-fast-json-stable-stringify
branch
from
bbe2fb3
to
ac74d8e
Compare
hwillson
approved these changes
May 17, 2021
benjamn
added a commit
that referenced
this pull request
May 18, 2021
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
added a commit
that referenced
this pull request
May 18, 2021
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.
This was referenced May 18, 2021
benjamn
added a commit
that referenced
this pull request
Jun 15, 2021
Another opportunity to apply @brainkim's suggestion from this comment: #8222 (comment)
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.
Fixes #8185, though I would say this removal is more for ESM-friendliness than for performance, as I explain below.
Although
fast-json-stable-stringifyhas done its job well, it only provides a"main"field in itspackage.jsonfile, pointing to a CommonJS entry point, and does not appear to export any ECMAScript modules.Thanks to our conversion/abandonment of
fast-json-stable-stringifyand other CommonJS-only npm dependencies (zen-observablein #5961 andgraphql-tagin #6074), it should now (after this PR is merged) be possible to load@apollo/client/corefrom an ESM-aware CDN like jsdelivr.net or jspm.io:If you put that script tag in an HTML file, or inject it into the DOM of any webpage, you will currently see this error:
This list of errors used to be longer, but thankfully the only package left is
fast-json-stable-stringify!Note that we're loading
@apollo/client/core@betahere, not@apollo/client@beta. The reason@apollo/clientitself is not yet ESM-ready is thatreactandreact-domare unfortunately also CommonJS-only dependencies, and@apollo/clientcurrently/regrettably re-exports utilities from@apollo/client/react. See #5541 for background on why this somewhat awkward package structure was originally necessary.If importing from
@apollo/client/coreis a burden or feels weird, please know that we are planning to make@apollo/clientsynonymous with@apollo/client/corein Apollo Client 4.0 (see #8190), along with making@apollo/client/reactsynonymous with the v3 API of@apollo/client. While we think React developers will have an easy time accommodating these changes (just find/replace@apollo/clientwith@apollo/client/react), it's clearly a major breaking change, so it will have to wait until AC4.