Skip to content

Require hook_dom in node_map components#62

Open
Copilot wants to merge 4 commits intomasterfrom
copilot/add-debug-assertion-node-map
Open

Require hook_dom in node_map components#62
Copilot wants to merge 4 commits intomasterfrom
copilot/add-debug-assertion-node-map

Conversation

Copy link
Contributor

Copilot AI commented Jan 22, 2026

  • Explore the codebase to understand the issue
  • Add debug assertion to enforce hook_dom in node_map item components
  • Update README documentation with the new requirement
  • Simplify node_map reordering code relying on the new rule
    • Removed instance_dom_last_get function
    • Removed instance_reinsert function
    • Simplified reordering to directly use child.dom
  • Remove redundant child.dom && checks now that hook_dom is enforced
  • All tests pass
Original prompt

This section details on the original issue you should resolve

<issue_title>Require hook_dom in node_map component</issue_title>
<issue_description>Currently, components given to node_map can contain multiple nodes without a wrapper element.
I think that this makes reordering in node_map way more complicated.
See if there is a stable and efficient way to still allow node_map components to not have a hook_dom. If you found one, abort and do nothing but commenting me your result.
Otherwise, add a debug assertion to the node_map handler enforcing its dynamic children to all have a direct dom element. If you did that, also mention it in the readme and make sure no test case or example violates this new rule.
See if the node_dom handler code could be simplified relying on the new rule.
</issue_description>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 2 commits January 22, 2026 19:36
…ering code

Co-authored-by: L3P3 <4629449+L3P3@users.noreply.github.com>
Co-authored-by: L3P3 <4629449+L3P3@users.noreply.github.com>
Copilot AI changed the title [WIP] Add debug assertion for node_map component children Require hook_dom in node_map components Jan 22, 2026
Copilot AI requested a review from L3P3 January 22, 2026 19:38
@L3P3 L3P3 marked this pull request as ready for review January 22, 2026 21:03
Copilot AI review requested due to automatic review settings January 22, 2026 21:03
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enforces that components used with node_map must call hook_dom() to define their root DOM element, enabling significant simplification of the reordering logic.

Changes:

  • Added debug assertion to catch missing hook_dom() calls in node_map components
  • Simplified reordering by replacing recursive DOM traversal with direct child.dom access
  • Removed ~50 lines of workaround code (instance_dom_last_get and instance_reinsert)
  • Added documentation to README explaining the new requirement

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/lui.js Added debug assertion for hook_dom requirement; simplified reordering logic; removed recursive DOM traversal functions
README.md Documented that node_map components must use hook_dom

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Owner

@L3P3 L3P3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not cleaned up enough.

src/lui.js Outdated
!child.dom &&
error('node_map item components must call hook_dom() to define their root DOM element');

child.dom &&
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition is not needed anymore, right?
Also look for other similar cases around here.
We rely on child.dom existing!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Removed all redundant child.dom && checks (lines 983, 991) and simplified the dom_first assignment (lines 1026-1030) since we now rely on child.dom always existing. Addressed in b916bd8.

Co-authored-by: L3P3 <4629449+L3P3@users.noreply.github.com>
Copilot AI requested a review from L3P3 January 22, 2026 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Require hook_dom in node_map component

3 participants