fix(angular): ensure table options updates are not missed during first mount window - #6534
fix(angular): ensure table options updates are not missed during first mount window#6534riccardoperra wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesAngular options synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit e8ebb4a
☁️ Nx Cloud last updated this comment at |
421a4c1 to
8b49bd6
Compare
riccardoperra
changed the title
8b49bd6 to
c4bb4a5
Compare
🚀 Changeset Version Preview1 package(s) bumped directly, 17 bumped as dependents. 🟩 Patch bumps
|
MILLERMARRU
left a comment
MILLERMARRU
left a comment
There was a problem hiding this comment.
The mechanism here is solid. Wrapping _options in computed() is what makes the previousOptions === currentOptions reference check meaningful in the first place, calling the raw factory twice always returns two different object literals, so without the memoization the comparison would never be equal and every effect run would apply. With computed(), a real signal change is what produces a new reference, so the effect's very first run can now tell "options actually changed since I captured the initial snapshot" from "nothing changed, this is just the effect settling," instead of unconditionally skipping the first run like isMount did. Traced the timeline in #6530 and this correctly handles the specific race described there, an update that lands in the gap between construction and the effect's first flush is no longer silently dropped.
Worth being precise about scope though, since the changeset and the linked issue don't quite line up. #6530 describes two problems: the primary one is lazySignalInitializer.ts's queueMicrotask eager-init firing before Angular's input bindings are applied, throwing an uncaught NG0950 in test environments with an async beforeEach. The isMount swallow is called out as a secondary issue, one that only surfaces once you work around the first one. This PR only touches injectTable.ts, lazySignalInitializer.ts isn't part of the diff, so it fixes proposal (3) from the issue but not proposal (1). The reported symptom, the uncaught NG0950 in TestBed suites, is still there after this merges. Worth linking as "part of #6530" rather than "closes #6530", or the eager-init half is going to look resolved when it isn't.
|
Just went through the new commit, this closes the gap I mentioned earlier. Traced through a few sequences by hand (options changing before the table is ever constructed, options changing right after) and the effect's |
2 participants
wip have to fix some tests
This pull request improves the reactivity and reliability of the
injectTablefunction in the Angular Table package by ensuring that options updates are not missed and by making the handling of options more robust. It also adds a regression test for a previously reported issue and includes a minor test cleanup.Reactivity and options handling improvements:
computedto wrap the options function ininjectTable, ensuring the latest options are always tracked reactively. [1] [2]injectTableto avoid missing the initial options update and to prevent unnecessary updates when options haven't changed. This fixes a bug where the first options update could be dropped.Testing improvements:
featuresin the options.Summary by CodeRabbit