Repository navigation
fix(tabs): don't throw when removed before firstUpdated() completes (#1472) - #1478
mayuriphad wants to merge 3 commits into
Conversation
Signed-off-by: mayuriphad <mayuriphad656@gmail.com>
The tab overflow and resize observers are created at the end of the async firstUpdated(), but disconnectedCallback() disconnected them unconditionally. Removing a pharos-tabs element before firstUpdated() finished threw 'Cannot read properties of undefined (reading "disconnect")'. Make the observers optional and disconnect them only if they exist. Fixes ithaka#1472 Signed-off-by: mayuriphad <mayuriphad656@gmail.com>
🦋 Changeset detectedLatest commit: f45fb10 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this comment.
🟡 Changes recommended
Guard post-await initialization after disconnection and update the regression test to cover the reported timing window.
1 open issue
What changed in this PR
This bug fix prevents pharos-tabs from throwing when disconnected before asynchronous firstUpdated() initialization completes.
Changes:
- Makes tab observers optional and disconnects them conditionally.
- Adds regression coverage for early removal.
- Adds a patch changeset for
@ithaka/pharos.
| File | Summary |
|---|---|
packages/pharos/src/components/tabs/pharos-tabs.ts |
Guards observer cleanup, but post-disconnect asynchronous initialization still requires cancellation or a connectivity guard. |
packages/pharos/src/components/tabs/pharos-tabs.test.ts |
Adds early-removal coverage, but does not exercise removal while firstUpdated() is suspended on child updates. |
.changeset/tabs-early-disconnect.md |
Records the patch release for the bug fix. |
Review effort: Lite
| // The observers are created at the end of the async firstUpdated(), | ||
| // so they don't exist yet if the element is removed before that. | ||
| this._tabOverflowObserver?.disconnect(); | ||
| this._tabListResizeObserver?.disconnect(); |
There was a problem hiding this comment.
Good catch, fixed in f45fb10. If the element is removed while firstUpdated() is still awaiting its children, the observers are no longer created for the detached element. When a removed element is attached again, connectedCallback() sets them up, and the setup is now idempotent. The new test removes the element while its first update is pending, checks that no observers exist after the update finishes, and checks that they come back on re-append. Lint and Test pass on my fork's verify run: https://github.com/mayuriphad/pharos/actions/runs/40176986005
…on reconnect Address review: when the element is removed while firstUpdated() is still awaiting its children, don't create observers for the detached element, since disconnectedCallback() has already run and would never disconnect them. Set them up again in connectedCallback() when a removed element is attached again, and make the setup idempotent. Signed-off-by: mayuriphad <mayuriphad656@gmail.com>
This change: (check at least one)
Is this a breaking change? (check one)
Is the: (complete all)
What does this change address?
Fixes #1472.
PharosTabs.disconnectedCallback()throwsTypeError: Cannot read properties of undefined (reading 'disconnect')when the element is removed before its asyncfirstUpdated()has finished.How does this change work?
_tabOverflowObserverand_tabListResizeObserverare only created at the end offirstUpdated(), after it has awaited every tab and panel. They are now typed as optional rather than definitely assigned (!).disconnectedCallback()disconnects them with optional chaining, so a quick removal (for example a framework re-render or av-ifthat flips) no longer throws.can be removed before its first update has finishedappends apharos-tabsand removes it straight away.Additional context
I checked this on my fork's CI with the
verifyworkflow. With only the new test commit, the Test job fails with theTypeErrorfromdisconnectedCallback(run). With the fix, Lint and Test both pass (run). Prettier passes with the repo's config. I added a patch changeset for@ithaka/pharos.