fix: Implement `isDeepEqual` manually #1686

rafaeelaudibert · 2025-01-24T03:52:06Z

We were using a built-in Node function, but that was silly because Node functions aren't available in the browser. Most bundlers could probably work that out, but Vite doesn't, so let's implement that ourselves and avoid requiring a bundler to sort this out.

Closes #1685

We were using a builtin Node function, but that was silly because Node functions aren't available in the browser. Most builder could probably work that out, but Vite doesn't, so let's implement that ourselves and avoid requiring a bundler to sort this out. Closes #1685

vercel · 2025-01-24T03:52:12Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
posthog-js	✅ Ready (Inspect)	Visit Preview	Jan 24, 2025 4:02pm

github-actions · 2025-01-24T03:56:18Z

Size Change: 0 B

Total Size: 3.28 MB

ℹ️ View Unchanged

Filename	Size
`dist/all-external-dependencies.js`	209 kB
`dist/array.full.es5.js`	266 kB
`dist/array.full.js`	370 kB
`dist/array.full.no-external.js`	369 kB
`dist/array.js`	182 kB
`dist/array.no-external.js`	180 kB
`dist/customizations.full.js`	13.8 kB
`dist/dead-clicks-autocapture.js`	14.4 kB
`dist/exception-autocapture.js`	9.48 kB
`dist/external-scripts-loader.js`	2.64 kB
`dist/main.js`	182 kB
`dist/module.full.js`	370 kB
`dist/module.full.no-external.js`	369 kB
`dist/module.js`	182 kB
`dist/module.no-external.js`	180 kB
`dist/recorder-v2.js`	117 kB
`dist/recorder.js`	117 kB
`dist/surveys-preview.js`	67.5 kB
`dist/surveys.js`	64.2 kB
`dist/tracing-headers.js`	1.76 kB
`dist/web-vitals.js`	10.4 kB

_{compressed-size-action}

pauldambra · 2025-01-24T08:01:22Z

.eslintrc.js

+        {
+            files: ['react/**/*.ts'],
+            rules: {
+                'posthog-js/no-direct-null-check': 'off',


why turn this off instead of using isNull?

Side note, but @pauldambra what's the background on us using isNull etc rather than ===? Browser compatibility? Bytes?

the bundle reduced by 1% when we switched to isNull and isUndefined compared to the straight comparison

ofc likely that the react stuff is being bundled and run through a minifier by someone else but consistency is probably better than varying

we use the checks a lot and...

the minified code has (something like) fe(x) at each call site

whereas the direct check is (something like) x === void 0 when minified

so once you call isUndefined more times than the character count of adding the declaration to the bundle you add roughly half as many characters by moving to a function

it's the same reason we have some global things as variables in the utils folder because then they'll minify and we have xY in the bundle not navigator

Thanks for the context! So if someone was experimenting with a new minifier, this is something they could change to see if it made a difference.

(I'd expect a good minifier to make this kind of change automatically but I probably am hoping for too much)

Hmm, in my mind I did that to avoid crossing the boundary from posthog-js/react to posthog-js. I now see that doesn't make sense because this is a single package for those who're installing it, so I'm happy to change :)

Ah.. fair.. yep, something to bear in my mind if/when we start splitting this up

So if someone was experimenting with a new minifier, this is something they could change to see if it made a difference.

that'd be a very good minifier... it's more of a compiler at that point 🤯

there's two sources of (avoidable) size in the repo IMO

unnecessary repetition of primitives - like this example

heavy use of classes and no minification of class fields/properties

react/src/context/PostHogProvider.tsx

react/src/context/__tests__/PostHogProvider.test.jsx

robbie-c · 2025-01-24T08:20:17Z

react/src/utils/object-utils.ts

@@ -0,0 +1,19 @@
+export function isDeepEqual(obj1: any, obj2: any): boolean {


What's the provenance of this? Is it directly copied from the node library? If you wrote or modified it, it needs tests.

I've attempted to write this function a few times and always been wrong, so would be interested in the answer to this!

we've had to handle recursive json in other places in the SDK too... which is why i wondered about json stringify comparison performance

robbie-c · 2025-01-24T08:21:21Z

react/src/utils/object-utils.ts

+    const keys1 = Object.keys(obj1)
+    const keys2 = Object.keys(obj2)
+
+    if (keys1.length !== keys2.length) return false


Suggested change

if (keys1.length !== keys2.length) return false

if (keys1.length !== keys2.length) {

return false

}

We generally prefer this style in the codebase. (I'm surprised prettier didn't change this!)

robbie-c · 2025-01-24T08:22:18Z

react/src/utils/object-utils.ts

+        if (!keys2.includes(key)) return false
+        if (!isDeepEqual(obj1[key], obj2[key])) return false


Suggested change

if (!keys2.includes(key)) return false

if (!isDeepEqual(obj1[key], obj2[key])) return false

if (!keys2.includes(key) || !isDeepEqual(obj1[key], obj2[key])) {

return false

}

react/src/context/__tests__/PostHogProvider.test.jsx

robbie-c · 2025-01-24T08:24:22Z

.eslintrc.js

+        {
+            files: ['react/**/*.ts'],
+            rules: {
+                'posthog-js/no-direct-null-check': 'off',


Side note, but @pauldambra what's the background on us using isNull etc rather than ===? Browser compatibility? Bytes?

react/src/utils/object-utils.ts

joshsny

Looks good, we need to be very careful with that deep equal function

react/src/utils/object-utils.ts

joshsny · 2025-01-24T08:44:18Z

react/src/utils/object-utils.ts

@@ -0,0 +1,19 @@
+export function isDeepEqual(obj1: any, obj2: any): boolean {


I've attempted to write this function a few times and always been wrong, so would be interested in the answer to this!

abhyuditjain · 2025-01-24T14:44:12Z

How about this:

export function isDeepEqual(obj1: any, obj2: any, visited = new WeakMap()): boolean {
    if (obj1 === obj2) return true;

    if (obj1 === null || typeof obj1 !== 'object' || obj2 === null || typeof obj2 !== 'object') {
        return false;
    }

    // check visited to avoid infinite recursion
    if (visited.has(obj1) && visited.get(obj1) === obj2) return true;
    visited.set(obj1, obj2);

    const keys1 = Object.keys(obj1);
    const keys2 = Object.keys(obj2);

    if (keys1.length !== keys2.length) return false;

    // set for O(1) lookups
    const keys2Set = new Set(keys2);

    for (const key of keys1) {
        if (!keys2Set.has(key) || !isDeepEqual(obj1[key], obj2[key], visited)) {
            return false;
        }
    }

    return true;
}

Avoids keys2.includes() for each key and has a visited set for circular references.

rafaeelaudibert · 2025-01-24T16:09:49Z

We'll have a follow-up PR moving this new helper function to the PosthogProvider file. It's good enough for our use-case - nobody should have huge/circular configs, so this is fine, but this function can't be used whenever we need a general deepCompare.

rafaeelaudibert added the bump patch Bump patch version when this PR gets merged label Jan 24, 2025

rafaeelaudibert requested a review from a team January 24, 2025 03:52

vercel bot deployed to Preview January 24, 2025 03:55 View deployment

pauldambra reviewed Jan 24, 2025

View reviewed changes

react/src/context/PostHogProvider.tsx Show resolved Hide resolved

pauldambra reviewed Jan 24, 2025

View reviewed changes

react/src/context/__tests__/PostHogProvider.test.jsx Show resolved Hide resolved

robbie-c reviewed Jan 24, 2025

View reviewed changes

pauldambra reviewed Jan 24, 2025

View reviewed changes

react/src/utils/object-utils.ts Show resolved Hide resolved

joshsny reviewed Jan 24, 2025

View reviewed changes

robbie-c added 2 commits January 24, 2025 15:51

Add some tests

539d240

Fix recursion test

56ddd00

vercel bot deployed to Preview January 24, 2025 15:58 View deployment

Couple more test cases

c7b47b1

vercel bot deployed to Preview January 24, 2025 16:02 View deployment

robbie-c approved these changes Jan 24, 2025

View reviewed changes

rafaeelaudibert merged commit 607a185 into main Jan 24, 2025
28 checks passed

rafaeelaudibert deleted the fix-strict-deep-equal branch January 24, 2025 16:09

robbie-c mentioned this pull request Jan 24, 2025

fix: Fix hitting call stack limit when deep comparing circular objects #1688

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Implement `isDeepEqual` manually #1686

fix: Implement `isDeepEqual` manually #1686

rafaeelaudibert commented Jan 24, 2025

vercel bot commented Jan 24, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading

pauldambra Jan 24, 2025

robbie-c Jan 24, 2025 •

edited

Loading

pauldambra Jan 24, 2025 •

edited

Loading

pauldambra Jan 24, 2025 •

edited

Loading

robbie-c Jan 24, 2025

rafaeelaudibert Jan 24, 2025

pauldambra Jan 24, 2025

pauldambra Jan 24, 2025

pauldambra Jan 24, 2025 •

edited

Loading

robbie-c Jan 24, 2025

joshsny Jan 24, 2025

pauldambra Jan 24, 2025

robbie-c Jan 24, 2025

robbie-c Jan 24, 2025

robbie-c Jan 24, 2025 •

edited

Loading

joshsny left a comment

joshsny Jan 24, 2025

abhyuditjain commented Jan 24, 2025

rafaeelaudibert commented Jan 24, 2025

		@@ -0,0 +1,19 @@
		export function isDeepEqual(obj1: any, obj2: any): boolean {

		if (!keys2.includes(key)) return false
		if (!isDeepEqual(obj1[key], obj2[key])) return false

fix: Implement isDeepEqual manually #1686

fix: Implement isDeepEqual manually #1686

Conversation

rafaeelaudibert commented Jan 24, 2025

vercel bot commented Jan 24, 2025 • edited Loading

github-actions bot commented Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

robbie-c Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

pauldambra Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

pauldambra Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pauldambra Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbie-c Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

joshsny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhyuditjain commented Jan 24, 2025

rafaeelaudibert commented Jan 24, 2025

fix: Implement `isDeepEqual` manually #1686

fix: Implement `isDeepEqual` manually #1686

vercel bot commented Jan 24, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading

robbie-c Jan 24, 2025 •

edited

Loading

pauldambra Jan 24, 2025 •

edited

Loading

pauldambra Jan 24, 2025 •

edited

Loading

pauldambra Jan 24, 2025 •

edited

Loading

robbie-c Jan 24, 2025 •

edited

Loading