Skip to content
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

Narrows type of parent in render functions. #3863

Merged
merged 4 commits into from
Jan 27, 2023

Conversation

MicahZoltu
Copy link
Contributor

https://gist.github.com/developit/f4c67a2ede71dc2fab7f357f39cff28c is the recommended way to partial root rendering in Preact 10+, but it isn't API compliant code given the current API definition as expressed by the TS types. This PR changes the type of parent to align with what is actually required by the preact render functions according to @developit.

Assuming the linked code actually works, then it means preact doesn't actually need a NodeList, but rather just needs an ArrayLike<Node>. Along with limiting the property set of Parent, I have also narrowed Parent['childNodes'] to be an ArrayLike<Node> so someone can assign a Node[] to it.

I suspect my stylistic choice of a single long line is not acceptable for this repository, but I'm not clear what the proper way to break this into multiple lines is for this repository. If someone can provide me with feedback on how to correctly split the lines, please let me know!

Note: I recognize that this is a pretty substantial type change, but without it the official recommendation (and only option in Preact 11) for doing partial root rendering is incorrect. Either the recommendation for how to do partial root rendering should be corrected to not lead people to an incorrect solution, or this PR (or one like it) should be merged to correctly express the type that preact expects for its render functions.

src/index.d.ts Outdated
@@ -275,9 +275,11 @@ export namespace h {
// Preact render
// -----------------------------------

type Parent = Omit<Pick<Element | Document | ShadowRoot | DocumentFragment, 'nodeType' | 'parentNode' | 'firstChild' | 'insertBefore' | 'appendChild' | 'removeChild'>, 'childNodes'> & { childNodes: ArrayLike<Node> }
Copy link
Member

Choose a reason for hiding this comment

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

Would it work if instead you did something like:

interface ContainerNode {
  nodeType: Node['nodeType'];
  parentNode: Node['nodeType'];
  firstChild: Node['firstchild'];
  insertBefore: Node['insertBefore'];
  appendChild: Node['appendChild'];
  removeChild: Node['removeChild'];
  childNodes: ArrayLike<Node>
}

I find this a bit easier to read and my understanding is that all the types (Element | Document | ShadowRoot | .. etc) already implement this interface so it should work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked on this and you are correct that all of these properties come from Node, so what you have proposed would work. I'll make this change to the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied the change. Let me know if there are other things I should change!

@coveralls
Copy link

coveralls commented Jan 25, 2023

Coverage Status

Coverage: 99.531%. Remained the same when pulling f91827b on MicahZoltu:patch-1 into c483d96 on preactjs:master.

@MicahZoltu
Copy link
Contributor Author

Hmm, the recent change proposed by @andrewiggins seems to have caused the tests to not compile, which surprises me a bit. I double checked the properties prior to applying the change and it looked like the types matched on all of them. I'll try to take a look at this and figure out why the tests are failing to compile.

Error: test/ts/preact.tsx(58,59): error TS2769: No overload matches this call.
  Overload 1 of 2, '(vnode: ComponentChild, parent: ContainerNode): void', gave the following error.
    Argument of type 'Document' is not assignable to parameter of type 'ContainerNode'.
      Types of property 'parentNode' are incompatible.
        Type 'ParentNode | null' is not assignable to type 'number'.
          Type 'null' is not assignable to type 'number'.
  Overload 2 of 2, '(vnode: ComponentChild, parent: ContainerNode, replaceNode?: Element | Text | undefined): void', gave the following error.
    Argument of type 'Document' is not assignable to parameter of type 'ContainerNode'.
Error: test/ts/preact.tsx(61,2): error TS2769: No overload matches this call.
  Overload 1 of 2, '(vnode: ComponentChild, parent: ContainerNode): void', gave the following error.
    Argument of type 'Document' is not assignable to parameter of type 'ContainerNode'.
  Overload 2 of 2, '(vnode: ComponentChild, parent: ContainerNode, replaceNode?: Element | Text | undefined): void', gave the following error.
    Argument of type 'Document' is not assignable to parameter of type 'ContainerNode'.
Error: test/ts/preact.tsx(69,2): error TS2769: No overload matches this call.
  Overload 1 of 2, '(vnode: ComponentChild, parent: ContainerNode): void', gave the following error.
    Argument of type 'Document' is not assignable to parameter of type 'ContainerNode'.
  Overload 2 of 2, '(vnode: ComponentChild, parent: ContainerNode, replaceNode?: Element | Text | undefined): void', gave the following error.
    Argument of type 'Document' is not assignable to parameter of type 'ContainerNode'.
Error: test/ts/preact.tsx(71,47): error TS2769: No overload matches this call.
  Overload 1 of 2, '(vnode: ComponentChild, parent: ContainerNode): void', gave the following error.
    Argument of type 'Document' is not assignable to parameter of type 'ContainerNode'.
  Overload 2 of 2, '(vnode: ComponentChild, parent: ContainerNode, replaceNode?: Element | Text | undefined): void', gave the following error.
    Argument of type 'Document' is not assignable to parameter of type 'ContainerNode'.
Error: test/ts/preact.tsx(72,68): error TS2769: No overload matches this call.
  Overload 1 of 2, '(vnode: ComponentChild, parent: ContainerNode): void', gave the following error.
    Argument of type 'Document' is not assignable to parameter of type 'ContainerNode'.
  Overload 2 of 2, '(vnode: ComponentChild, parent: ContainerNode, replaceNode?: Element | Text | undefined): void', gave the following error.
    Argument of type 'Document' is not assignable to parameter of type 'ContainerNode'.
Error: test/ts/preact.tsx(79,2): error TS2769: No overload matches this call.
  Overload 1 of 2, '(vnode: ComponentChild, parent: ContainerNode): void', gave the following error.
    Argument of type 'Document' is not assignable to parameter of type 'ContainerNode'.
  Overload 2 of 2, '(vnode: ComponentChild, parent: ContainerNode, replaceNode?: Element | Text | undefined): void', gave the following error.
    Argument of type 'Document' is not assignable to parameter of type 'ContainerNode'.

Copy link
Member

@andrewiggins andrewiggins left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Looks like we are close :)

src/index.d.ts Outdated Show resolved Hide resolved
MicahZoltu and others added 4 commits January 27, 2023 16:05
https://gist.github.com/developit/f4c67a2ede71dc2fab7f357f39cff28c is the recommended way to partial root rendering in Preact 10+, but it isn't API compliant code given the current API definition as expressed by the TS types.
This PR changes the type of `parent` to align with what is *actually* required by the preact render functions according to @developit.

Assuming the linked code actually works, then it means preact doesn't actually need a NodeList, but rather just needs an `ArrayLike<Node>`.
Along with limiting the property set of `Parent`, I have also narrowed `Parent['childNodes']` to be an `ArrayLike<Node>` so someone can assign a `Node[]` to it.

I suspect my stylistic choice of a single long line is not acceptable for this repository, but I'm not clear what the proper way to break this into multiple lines is for this repository.  If someone can provide me with feedback on how to correctly split the lines, please let me know!

Note: I recognize that this is a pretty substantial type change, but without it the official recommendation (and only option in Preact 11) for doing partial root rendering is incorrect.
Either the recommendation for how to do partial root rendering should be corrected to not lead people to an incorrect solution, or this PR (or one like it) should be merged to correctly express the type that preact expects for its `render` functions.
Co-authored-by: Andre Wiggins <[email protected]>
Copy link
Member

@andrewiggins andrewiggins left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you <3

@andrewiggins andrewiggins merged commit 15913ad into preactjs:master Jan 27, 2023
@MicahZoltu MicahZoltu deleted the patch-1 branch January 28, 2023 04:46
JoviDeCroock added a commit that referenced this pull request Jan 12, 2024
JoviDeCroock added a commit that referenced this pull request Jan 12, 2024
* backport #3871

* port test from #3884 functionality seems to work in v11

* backport #3880

* backport #3875

* backport #3862

* add todo for #3801

* backport #3868

* backport #3867

* backport #3863

* add todo for #3856

* backport #3844

* backport #3816

* backport #3888

* backport #3889
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.

None yet

3 participants