Skip to content

ChildNode.nextSibling should be another ChildNode, not just a Node #28551

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jcormont opened this issue Nov 15, 2018 · 9 comments
Closed

ChildNode.nextSibling should be another ChildNode, not just a Node #28551

jcormont opened this issue Nov 15, 2018 · 9 comments
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Help Wanted You can do this
Milestone

Comments

@jcormont
Copy link

TypeScript Version: 3.1.6

Search Terms: ChildNode Node nextSibling

Code

// a basic loop over elements in the DOM
let element = document.body.firstChild;  // is a ChildNode | null
while (element) {
    // ... do something with element
    let sibling = element.nextSibling;  // is a Node | null -- why?
    element = sibling;  // uh-uh!
}

Expected behavior:

Surely a sibling of a child is itself also a child (of the same parent). So siblings should be assignable using the same variable.

Actual behavior:

Error:

Type 'Node | null' is not assignable to type 'ChildNode | null'.
  Type 'Node' is not assignable to type 'ChildNode'.
    Property 'after' is missing in type 'Node'. [2322]

Related Issues:

I think this is what #27453 also refers to, but fails to describe accurately so it has been closed.

In any case this is related to the 'solution' in #24633 which clearly shows the type of nextSibling as Node, not ChildNode.

@weswigham weswigham added Bug A bug in TypeScript Help Wanted You can do this Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript labels Nov 15, 2018
@weswigham
Copy link
Member

PRs welcomed. You can find more information about contributing lib.d.ts fixes at https://github.com/Microsoft/TypeScript/blob/master/CONTRIBUTING.md#contributing-libdts-fixes.

@saschanaz
Copy link
Contributor

Current ChildNode is a hack, it should really be a union of DocumentType | Element | CharacterData.

@Fabianopb
Copy link

Updating nextSibling: Node | null to nextSibling: ChildNode | null sounds reasonable.

ChildNode extends Node (which extends EventTarget) so I imagine it won't be a breaking change, just adding the correct type. @saschanaz why do you think ChildNode should be this union type you mentioned?

Btw, I'd be glad to grab this story if you guys haven't started yet!

@Fabianopb
Copy link

PR opened, maybe you guys want to continue the conversation in there.

@saschanaz
Copy link
Contributor

saschanaz commented Nov 20, 2018

why do you think ChildNode should be this union type you mentioned?

No, I think the type of firstChild property should be, not ChildNode itself. ChildNode is by the DOM spec a mixin that adds some child-node-only methods and should only be used in extends part.

microsoft/TypeScript-DOM-lib-generator@5cd35f3 introduced the deviation.

@Fabianopb
Copy link

I wonder what was the reason then to make ChildNode extend Node in that commit. As a comparison, ParentNode seems by the spec. Maybe @mhegazy remembers what was the story?

@saschanaz
Copy link
Contributor

I wonder what was the reason then to make ChildNode extend Node in that commit.

I think he just wanted to remove Node & ChildNode.

@Fabianopb
Copy link

microsoft/TypeScript-DOM-lib-generator#619 is merged, should this be closed?

@jakebailey
Copy link
Member

Just looking at ancient issues; yes, the above was merged and then brought over in #29690.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Help Wanted You can do this
Projects
None yet
Development

No branches or pull requests

6 participants