Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses two important issues in the reactive system for custom elements:
-
Reactive root per element: Previously, child elements created in a parent's template would share the parent's reactive root, causing their effects to be tied to the parent's lifecycle. This prevented proper cleanup when the child element was disconnected independently of the parent.
-
Solid.js bug workaround: Works around a Solid.js issue (solidjs/solid#2580) where effects created during component render may never run due to reactive tracking issues.
Changes:
- Added
untrackwrapper aroundsuper.connectedCallback()in the element decorator to prevent unwanted reactive tracking - Implemented per-element reactive roots using Solid.js
createRoot,getOwner, andrunWithOwnerAPIs - Added proper disposal of reactive roots in
disconnectedCallback - Removed debug
console.logfrom attribute test
Reviewed changes
Copilot reviewed 5 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/decorators/element.ts | Added untrack workaround in connectedCallback override to prevent Solid.js bug |
| src/LumeElement.ts | Implemented per-element reactive root creation, storage, and disposal; wrapped effect starts in runWithOwner |
| src/decorators/element.test.ts | Added test case for Solid.js bug workaround with createSignal and render |
| src/decorators/attribute.test.ts | Removed debug console.log statement |
| src/LumeElement.test.ts | Added test for independent effect lifecycle management of child elements |
| dist/* | Generated build artifacts reflecting source changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
cb39144 to
4eed14d
Compare
see Copilot's description below