fix: fire computed listeners for Temporal values#656
Merged
jmeistrich merged 2 commits intoJul 4, 2026
Conversation
Temporal.* instances have no enumerable own properties, so the change diff in updateNodesAndNotify treated them as empty objects and skipped notifications, mirroring the same issue Date already handles. Add an isTemporal guard (duck-typed via Symbol.toStringTag so it works without a global Temporal) and treat Temporal instances as opaque, primitive-like values compared by their string representation, the same way Date is compared by timestamp. Fixes LegendApp#650
jmeistrich
requested changes
Jul 4, 2026
jmeistrich
left a comment
jmeistrich
left a comment
Member
There was a problem hiding this comment.
I think this looks good! Just one small tweak to a test and I think we can merge :)
| expect(handler).toHaveBeenCalledTimes(1); | ||
|
|
||
| // Setting to a string that resolves to an equal PlainDate should not re-notify. | ||
| source$.set('2024-01-15'); |
Member
There was a problem hiding this comment.
It looks like this is the same as the original string on line 295, should be different to make sure it's testing correctly?
Addresses review feedback on LegendApp#650: the equal-PlainDate test now sets a different string that still resolves to the same PlainDate, so it exercises value-equality rather than raw-string identity.
Contributor
Author
|
Good catch. Updated line 306 to set |
jmeistrich
approved these changes
Jul 4, 2026
Member
|
Merged! Thank you! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Computed observables that wrap a
Temporal.*value never fire their listeners on change (#650). Temporal instances have no enumerable own properties, so the change-diff treats them as empty objects and decides nothing changed.Temporalvalues are now detected (duck-typed viaSymbol.toStringTag, which every Temporal type sets to"Temporal.<Type>", so it works whether the runtime has native Temporal or a polyfill) and treated as opaque primitive-like values compared by their string form, mirroring the existingDatehandling. Wired into the sameisObject/isPrimitive/equalsspots that already special-caseDate.Added tests: a computed returning a
Temporal.PlainDate/Instantnotifies on change and does not notify when the value is equal. They fail without the fix (the listener fires once instead of twice) and pass with it.@js-temporal/polyfillis added as a dev-only dependency so the tests can run on Node, which has no native Temporal yet. tsc, eslint and prettier clean.Closes #650