Fix detached node morphing and cover edge cases#43
Conversation
There was a problem hiding this comment.
Pull request overview
This PR targets hard-to-hit edge cases in Morphlex’s DOM morphing logic (notably around detached nodes) and adds regression tests to reach full line/branch coverage.
Changes:
- Adjust morphing internals to better handle detached nodes during replacement and one-to-many morphs.
- Refactor ID-mapping/dirty-flagging helpers to operate on
Elementnodes (viaisElementNode), and simplify a few matching paths. - Add a dedicated regression test suite for previously uncovered/edge scenarios, plus V8 coverage ignores where needed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/morphlex.ts |
Fixes detached-node handling and refactors node-type helpers; adds a few V8 ignore annotations. |
test/new/coverage.test.ts |
Adds targeted regression tests covering detached morphs, callback behavior, id matching, whitespace, and textarea/option edge cases. |
test/new/utils.ts |
Marks helper utilities file as ignored for V8 coverage reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (from.nodeType === to.nodeType && from.nodeValue !== null && to.nodeValue !== null) { | ||
| if (from.nodeValue !== to.nodeValue) { | ||
| from.nodeValue = to.nodeValue | ||
| } | ||
| from.nodeValue = to.nodeValue | ||
| } else { |
There was a problem hiding this comment.
In #morphOtherNode, from.nodeValue is assigned unconditionally whenever node types match. Even when the value is unchanged, this can generate unnecessary DOM mutations (and potentially extra MutationObserver characterData records), and it removes a previous micro-optimization. Consider restoring the if (from.nodeValue !== to.nodeValue) guard so identical text/comment nodes don’t incur a write.
| #replaceNode(node: ChildNode, newNode: ChildNode): void { | ||
| const parent = node.parentNode || document | ||
| const parent = node.parentNode | ||
| const callbackParent = parent || document | ||
| const insertionPoint = node | ||
| // Check if both removal and addition are allowed before starting the replacement | ||
| if ( | ||
| (this.#options.beforeNodeRemoved?.(node) ?? true) && | ||
| (this.#options.beforeNodeAdded?.(parent, newNode, insertionPoint) ?? true) | ||
| (this.#options.beforeNodeAdded?.(callbackParent, newNode, insertionPoint) ?? true) | ||
| ) { | ||
| if (!parent) return | ||
| parent.insertBefore(newNode, insertionPoint) |
There was a problem hiding this comment.
#replaceNode may invoke beforeNodeRemoved/beforeNodeAdded for a detached node (no parentNode), then return early without actually inserting/removing anything. This breaks the stated callback contract (“before removed/added to the DOM”) and can cause user code to run side effects assuming a real replacement will occur. Consider short-circuiting when parent is null (or at least skipping beforeNodeRemoved / after* callbacks) while still allowing the detached-parent behavior you want for beforeNodeAdded (e.g., call beforeNodeAdded(document, ...) but avoid calling beforeNodeRemoved when nothing can be removed).
This fixes an extreme edge case and brings us up to 100% line and branch coverage.