Skip to content

Include Comment nodes in polyfill hook types - #691

Open
olavoasantos wants to merge 1 commit into
normalize-text-node-hooksfrom
type-comment-hooks
Open

olavoasantos wants to merge 1 commit into
normalize-text-node-hooksfrom
type-comment-hooks

Conversation

@olavoasantos

@olavoasantos olavoasantos commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Problem

The polyfill invokes text and child-mutation hooks for Comment nodes at runtime, but the corresponding Hooks TypeScript signatures accepted only Text nodes. The public type contract therefore rejected a runtime-supported node kind.

Impact

Minor. TypeScript consumers implementing setText, insertChild, or removeChild could not accurately type their handling of comments. This is type-contract drift rather than a runtime failure, but it can force unsafe casts or make valid hook implementations fail type checking.

Reproduction

const comment = document.createComment('Comment');

parent.appendChild(comment);
comment.data = 'Updated comment';
parent.removeChild(comment);

The added regression test verifies that runtime calls already pass the Comment node to insertChild, setText, and removeChild. Before this change, the type-level assertions that a Document['createComment'] result is accepted by those hook parameter types failed; they now pass.

Change

Widen the affected hook parameters from Text to Text | Comment, matching the nodes the polyfill already supplies at runtime. No hook dispatch behavior changes.

Tests

Adds compile-time assertions for comment compatibility with all three hook signatures, plus runtime spies that verify insertion, text updates, and removal receive the comment node and expected payloads.

Stack

Validation

Fresh GitHub CI on restacked head 08e3223 passes:

  • lint;
  • type-check and build;
  • unit tests with coverage;
  • bundle-size checks;
  • Playwright end-to-end tests;
  • the classified Web Platform Test suite.

@olavoasantos
olavoasantos marked this pull request as ready for review September 4, 2026 15:29
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.

4 participants