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

Decouple canonicalStringify from ObjectCanon #11254

Merged
merged 18 commits into from
Oct 5, 2023

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Sep 26, 2023

Because the ObjectCanon happens to produce canonical objects whose keys have been sorted alphabetically/lexicographically, it seemed convenient and efficient for implementing a stable JSON.stringify utility (see PR #8222), but the additional memory overhead of fully canonizing (duplicating) the objects was probably not justified, especially since (as this PR suggests) an independent implementation of canonicalStringify can be even faster while using much less memory.

@benjamn benjamn requested a review from phryneas September 26, 2023 18:27
@benjamn benjamn self-assigned this Sep 26, 2023
@changeset-bot
Copy link

changeset-bot bot commented Sep 26, 2023

🦋 Changeset detected

Latest commit: d0aa030

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2023

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 37.14 KB (+0.08% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 43.5 KB (+0.08% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 42.03 KB (+0.04% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 32.57 KB (+0.1% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 31.31 KB (+0.1% 🔺)
import { ApolloProvider } from "dist/react/index.js" 1.22 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production) 1.21 KB (0%)
import { useQuery } from "dist/react/index.js" 4.28 KB (0%)
import { useQuery } from "dist/react/index.js" (production) 4.09 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 4.59 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production) 4.41 KB (0%)
import { useMutation } from "dist/react/index.js" 2.53 KB (0%)
import { useMutation } from "dist/react/index.js" (production) 2.51 KB (0%)
import { useSubscription } from "dist/react/index.js" 2.24 KB (0%)
import { useSubscription } from "dist/react/index.js" (production) 2.2 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 4.25 KB (-7.58% 🔽)
import { useSuspenseQuery } from "dist/react/index.js" (production) 3.69 KB (-8.3% 🔽)
import { useBackgroundQuery } from "dist/react/index.js" 3.73 KB (-9.32% 🔽)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.18 KB (-9.97% 🔽)
import { useReadQuery } from "dist/react/index.js" 2.97 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production) 2.92 KB (0%)
import { useFragment } from "dist/react/index.js" 2.08 KB (0%)
import { useFragment } from "dist/react/index.js" (production) 2.03 KB (0%)

@phryneas
Copy link
Member

/release:pr

@github-actions
Copy link
Contributor

A new release has been made for this PR. You can install it with npm i @apollo/[email protected].

Comment on lines 74 to 77
// The .slice(0) is necessary so that we do not modify the original keys array
// by calling keys.sort(), and also so that we always return a new (!==)
// sorted array when keys was not already sorted.
return node.sorted || (node.sorted = keys.slice(0).sort());
Copy link
Member Author

Choose a reason for hiding this comment

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

Fun TC39/ECMAScript fact: thanks to this proposal which recently reached stage 4, we will eventually be able to use keys.toSorted() instead of keys.slice(0).sort(). "Eventually" because even evergreen browsers take some time to ship new features.

Copy link
Member

Choose a reason for hiding this comment

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

At least it's one of those things that can easily be polyfilled in userland, so we can make that switch in two years or so :)

@benjamn benjamn force-pushed the decouple-canonicalStringify-from-ObjectCanon branch from 4f3d137 to 9211fd4 Compare September 27, 2023 17:21
@benjamn benjamn force-pushed the decouple-canonicalStringify-from-ObjectCanon branch from d7d190f to 1b4aad3 Compare September 28, 2023 18:10
Copy link
Member

@phryneas phryneas left a comment

Choose a reason for hiding this comment

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

This is gonna have a big impact, especially in SSR scenarios.
Thanks for taking the time to go over this!

@benjamn benjamn force-pushed the decouple-canonicalStringify-from-ObjectCanon branch from cfb5b42 to d0aa030 Compare October 2, 2023 15:27
Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

This looks great to me! Nothing to add that @phryneas hasn't already. 🎉

@phryneas
Copy link
Member

phryneas commented Oct 5, 2023

Then let's get this into the 3.9 branch so we can have a few people try it out soon :)

@phryneas phryneas merged commit d08970d into release-3.9 Oct 5, 2023
5 checks passed
@phryneas phryneas deleted the decouple-canonicalStringify-from-ObjectCanon branch October 5, 2023 08:05
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2023
@phryneas phryneas modified the milestones: Release 3.9, MemoryAnalysis Nov 9, 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