Repository navigation
Conversation
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
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.
ObservableHint.opaquehas no effect on arrays.updateNodesand the proxygettrap only check the hint behindisObject(), which excludes arrays. An opaque-hinted array is still walked and gets child nodes for every element.The types already treat an opaque array as a primitive. In
observable({ arr: ObservableHint.opaque([...]) }),arr[0]andarr.pushdon't type-check, so the runtime was the only part that disagreed.Change
Check the opaque hint for arrays too, in the same
(isObject(x) || isArray(x))form thatextractFunctionsAndComputedsalready uses. Setting an opaque array now notifies listeners once and creates no child nodes. Reading an index through the proxy returns the raw element, like a property of an opaque object.Why it matters
We hit this in a React Native app. It stored a list of Relay fragment refs with
ObservableHint.opaque(messages). Each ref carries the Relay query AST, so every element grew about 1,500 child nodes. With ~1,000 loaded messages that was ~400 MB of JS heap, and eachsetre-diffed the whole tree, taking up to ~250 ms. That was enough to crash the app on long lists.Testing
opaque array should be handled like primitives. It fails onmain(the node has children) and passes with this change.tsc --noEmit, ESLint and Prettier pass on the changed lines.bun test --timeout 50has the same 19 failures as unmodifiedmain, mostly sync-retry tests that run past the 50 ms timeout, so nothing new.