-
Notifications
You must be signed in to change notification settings - Fork 77
fix: handle Temporal objects correctly in proxy deepClone and deepEqual #434
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
Conversation
🦋 Changeset detectedLatest commit: 1aed4aa The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
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 |
More templates
@tanstack/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
Size Change: +74 B (+0.11%) Total Size: 64.6 kB
ℹ️ View Unchanged
|
Size Change: 0 B Total Size: 1.16 kB ℹ️ View Unchanged
|
This works beautifully. Thanks v much for jumping on this so quickly 🏎️ |
Actually, I spoke too soon 😓 Unless I'm doing something wrong, I'm seeing some different odd behaviour now, and I put together this little reproduction. If you click the button after the page loads to mutate the data in the store, you will see that the data that's returned from the live query gets out of sync with the data retrieved directly from the store, even though a refetch is happening. |
Ok interesting — thanks for the further reproduction! Me or someone else will look into this next week. |
Thanks, I appreciate it. Looks like the code in db/packages/db/src/collection.ts Lines 1712 to 1733 in 25c43cd
|
Ah nice! |
I took a stab at fixing this, and here's a commit you can cherry-pick onto this branch (if you agree with it!) that makes the issues I was seeing go away: obeattie@c7cf39c I noticed there are a bunch of different functions for checking deep equality, so I combined them in that commit. |
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.
I had a look at this PR and left some comments that need fixing. Nothing major, just refactorings to avoid code duplication. Still need to look into @obeattie's commit.
packages/db/src/proxy.ts
Outdated
`Temporal.PlainYearMonth`, | ||
`Temporal.PlainMonthDay`, | ||
`Temporal.Duration`, | ||
`Temporal.TimeZone`, |
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.
These 2 temporals (TimeZone
and Calendar
) don't seem to exist according to the docs: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal
packages/db/src/proxy.ts
Outdated
@@ -133,6 +133,30 @@ function deepClone<T extends unknown>( | |||
return clone as unknown as T | |||
} | |||
|
|||
// Handle Temporal objects | |||
// Check if it's a Temporal object by checking for the Temporal brand | |||
if (typeof (obj as any)[Symbol.toStringTag] === `string`) { |
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.
We should extract the check to a helper function isTemporalObject
that does this logic of checking the toStringTag
. We could also use a plain instanceof
check unless these temporals may come from a different realm (not sure?).
packages/db/src/proxy.ts
Outdated
const aTag = (a as any)[Symbol.toStringTag] | ||
const bTag = (b as any)[Symbol.toStringTag] | ||
|
||
if (typeof aTag === `string` && typeof bTag === `string`) { |
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.
Let's not duplicate this temporal checking logic. We should use a helper function.
packages/db/src/proxy.ts
Outdated
|
||
if (aIsTemporal && bIsTemporal) { | ||
// If they're different Temporal types, they're not equal | ||
if (aTag !== bTag) return false |
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.
Let's also make a helper function to get the tag of a temporal
packages/db/src/proxy.ts
Outdated
} | ||
|
||
// For Duration, use toString comparison as it doesn't have equals | ||
if (aTag === `Temporal.Duration`) { |
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.
Since the default is to fallback to toString
comparison (L308) there's no need for this if test.
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.
Actually, i wouldn't fallback to string comparison here but rather use the static compare
method: Temporal.Duration.compare(a, b) === 0
packages/db/src/proxy.ts
Outdated
@@ -741,12 +806,31 @@ export function createChangeProxy< | |||
return value.bind(ptarget) | |||
} | |||
|
|||
// If the value is an object, create a proxy for it | |||
// Check if it's a Temporal object - don't proxy them as they're immutable | |||
const isTemporalObject = |
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.
Use a helper function instead of duplicating this code.
@obeattie Your commit is spot on! As far as i can tell, you replaced the different versions of Thanks for jumping in and fixing this 👍 |
@obeattie I cherry-picked your commit and i further cleaned this PR. Your stackblitz example now works correctly. Before i merge this I'd like to add a unit test though that reproduces the original bug to avoid future regressions. |
Thanks for picking this up @kevin-dp!
Yes that's exactly right. In the longer term, and as I think is touched upon in #428's discussion, I think it might be good to allow users to provide their own comparators for different types. If that functionality existed I would probably have just used that, though it's also easy to make a case that Appreciate you both for fixing this so quickly! |
The proxy's deepClone function was attempting to clone Temporal objects (like Temporal.ZonedDateTime) as regular objects, resulting in empty objects being produced. This fix: - Detects Temporal objects by their Symbol.toStringTag - Returns Temporal objects directly from deepClone (they're immutable) - Adds proper equality checking for Temporal objects in deepEqual - Prevents proxy creation for Temporal objects in the get handler - Adds test coverage for Temporal.ZonedDateTime, PlainDate, and Duration Fixes the issue where Temporal objects were being converted to empty objects during proxy tracking operations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Consolidate multiple deepEquals implementations into a single function in utils.ts - Remove deepEqual functions from collection.ts and proxy.ts - Add tests
I added a unit test that reproduces the bug when ran before @obeattie's commit and passes after that commit. All good on my side to merge it. I'll wait untill this afternoon to give @KyleAMathews a chance to have a look at it. |
Looks great! I found another bug around live queries not updating when I modified a (regular) date which this I tested this PR on and it also fixed so glad you cleaned up our sprawling deep equals mess @obeattie :-D Let's get this out there |
Summary
Temporal.ZonedDateTime
) were being converted to empty objects during proxy tracking operationsdeepClone
anddeepEqual
functionsProblem
When using Temporal objects with TanStack DB's proxy system, the
deepClone
function was attempting to clone Temporal objects as regular objects by copying their enumerable properties. Since Temporal objects store their data internally (not as enumerable properties), this resulted in empty objects being produced.Solution
This PR adds special handling for Temporal objects:
deepClone
: Detects Temporal objects by theirSymbol.toStringTag
and returns them directly (since they're immutable)deepEqual
: Adds proper equality checking using Temporal's built-inequals()
method where availableTest plan
Temporal.ZonedDateTime
,Temporal.PlainDate
, andTemporal.Duration
🤖 Generated with Claude Code