-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
handle Symbol properties and non-enumerable properties in replaceEqualDeep
#4237
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
handle Symbol properties and non-enumerable properties in replaceEqualDeep
#4237
Conversation
View your CI Pipeline Execution ↗ for commit b248312.
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/history
@tanstack/eslint-plugin-router
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-with-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-config
@tanstack/react-start-plugin
@tanstack/react-start-router-manifest
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-config
@tanstack/solid-start-plugin
@tanstack/solid-start-router-manifest
@tanstack/solid-start-server
@tanstack/start
@tanstack/start-api-routes
@tanstack/start-client-core
@tanstack/start-config
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/start-server-functions-handler
@tanstack/start-server-functions-ssr
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
// without this PR: | ||
// expect(result).toStrictEqual({ a: 3 }) | ||
// with this PR: | ||
expect(result).toStrictEqual(obj2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without this PR:
expect(result).toStrictEqual({ a: 3 })
// without this PR: | ||
// expect(result).toBe(obj1) | ||
// with this PR: | ||
expect(result).toStrictEqual(obj2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without this PR:
expect(result).toBe(obj1)
// expect(result).toBe(obj1) | ||
// expect(result.b).toBe(2) | ||
// with this PR: | ||
expect(result).toBe(obj2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without this PR:
expect(result).toBe(obj1)
expect(result.b).toBe(2)
// expect(result).toStrictEqual({ a: 3 }) | ||
// expect(result.b).toBe(undefined) | ||
// with this PR: | ||
expect(result).toBe(obj2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without this PR:
expect(result).toStrictEqual({ a: 3 })
expect(result.b).toBe(undefined)
## Description Improve performance of this function that gets called a lot for structural sharing. Main improvements are - avoid creating object when possible (`Object.keys().concat(Object.getOwnPropertySymbols())` creates 3 arrays, when we only want 1) - instantiating array to the correct size avoids a lot of memory management under the hood (prefer `new Array(size)` over `[]`) - avoid reading the same value many times on an object, store is as a const More minor changes (not 100% sure I can measure it, but I think so): - using `keys.includes(k)` is slower than `Object.hasOwnProperty.call(obj, k)` or `obj.hasOwnProperty(k)` ## benchmark TL;DR: consistently 1.3x faster implementation ```ts describe('replaceEqualDeep', () => { bench('old implementation', () => { replaceEqualDeepOld({ a: 1, b: [2, 3] }, { a: 1, b: [2, 3] }) replaceEqualDeepOld({ a: 1, b: [2, 3] }, { a: 2, b: [2] }) }) bench('new implementation', () => { replaceEqualDeep({ a: 1, b: [2, 3] }, { a: 1, b: [2, 3] }) replaceEqualDeep({ a: 1, b: [2, 3] }, { a: 2, b: [2] }) }) }) ``` ```sh ✓ @tanstack/router-core tests/utils.bench.ts > replaceEqualDeep 1540ms name hz min max mean p75 p99 p995 p999 rme samples · old implementation 1,040,201.62 0.0008 0.7638 0.0010 0.0010 0.0013 0.0016 0.0022 ±0.33% 520101 · new implementation 1,347,988.70 0.0006 2.4037 0.0007 0.0007 0.0010 0.0010 0.0013 ±0.95% 673995 fastest BENCH Summary @tanstack/router-core new implementation - tests/utils.bench.ts > replaceEqualDeep 1.30x faster than old implementation ``` --- The `replaceEqualDeep` implementation before this PR handles Symbol keys, and non-enumerable keys (see #4237), but **not** keys that are both a Symbol and non-enumerable. This PR fixes this issue. But if we also fix it in the previous implementation before comparing performance we get a bigger perf diff ```sh ✓ @tanstack/router-core tests/utils.bench.ts > replaceEqualDeep 1471ms name hz min max mean p75 p99 p995 p999 rme samples · old implementation 713,964.88 0.0012 0.7880 0.0014 0.0014 0.0019 0.0023 0.0050 ±0.35% 356983 · new implementation 1,319,003.07 0.0006 5.0000 0.0008 0.0007 0.0010 0.0016 0.0050 ±1.96% 659502 fastest BENCH Summary @tanstack/router-core new implementation - tests/utils.bench.ts > replaceEqualDeep 1.85x faster than old implementation ``` --- <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Change detection now includes symbol-keyed properties, treats undefined/missing entries consistently, and avoids processing objects with non-enumerable own properties to prevent incorrect updates. * No public API changes. * **Performance** * Equality/merge logic reuses unchanged values more reliably and improves array update handling by allocating appropriately, reducing unnecessary work. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Opening this PR for discussion, I'm very open to scope changes.
This PR tackles two problems with
defaultStructuralSharing: true
:Point 1. actually is causing some of our users a headache, see apollographql/apollo-client#12619 and https://community.apollographql.com/t/preloadquery-with-tanstack-router-loader/9100 .
Users expect that they're able to pass an object with symbol properties over from a loader into components, but when having
defaultStructuralSharing
enabled, from the second object that is passed over, it creates a copy and strips off symbol properties.That new object without the expected symbol properties will then crash our
useReadQuery
hook.In other words, the first loader call returns
the component receives
and the second loader call returns
but the component receives
toPromise
is a different function, so it goes into the "clone" path, but skips thesecretSymbol
property.If we wouldn't have that
toPromise
property on there (and we're considering removing it!), it would even be worse to debug - users would always stay on the firstrefObject
and never get the second, since both objects would be considered equal.On our side, passing these objects from loaders to components is actually an intended pattern, so this is causing quite a bit of problems - see this example from our docs (see Initiating queries outside React):
Problem 2. is purely hypothetical and we haven't encountered it - but it should probably be handled in some way.
Both of these problems can be handled in two ways:
isPlainObject
just returnfalse
if an object has non-enumerable or symbol properties. This would opt out of structural sharing for these objects (which is probably perfectly fine)I'd be open for either of those solutions, and also for completely dropping the "non-enumerable" case for simplicity, but I would be very happy if we could get something in to help with symbol properties.
In React Query, these values can be assumed to be serializable JSON, so the current implementation is perfectly fine, but with client-side loaders, users can just pass anything, and here it's causing quite the problem.
Telling our users to situationally turn off structural sharing would be an educational nightmare.