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

ChildNode/ParentNode no longer extending Node #1087

Closed
DanielRosenwasser opened this issue Aug 2, 2021 · 8 comments · Fixed by #1088
Closed

ChildNode/ParentNode no longer extending Node #1087

DanielRosenwasser opened this issue Aug 2, 2021 · 8 comments · Fixed by #1088

Comments

@DanielRosenwasser
Copy link
Member

According to microsoft/TypeScript#45266, there are some breaks around ChildNode no longer being a subtype of Node. This was also mentioned in Google's 4.4 beta adoption thread (#1067).

Some breaks here

ant-design/ant-design

tsconfig.json

sindresorhus/refined-github

tsconfig.json

In #1067, @saschanaz explained

It's a mixin interface implemented by some Node interfaces. The correct way to receive Nodes that can be children is ChildNode & Node, just like ParentNode & Node for Nodes that can be parents.

I may be missing some context of why that's important to have - was there discussion around that? But regardless, I don't know if we can retroactively make that call. If that's a thing users need, does it make sense to create a ChildNodeBase and a ParentNodeBase?

@saschanaz
Copy link
Contributor

saschanaz commented Aug 2, 2021

Adding type ChildNodeBase = ChildNode & Node etc. makes sense to me.

I may be missing some context of why that's important to have - was there discussion around that?

TBH that was one of the changes back in the experimental types-web era where I wanted to be bold and more spec-compliant 🤔

Restoring the previous ChildNode is also an option.

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Aug 2, 2021

Adding type ChildNodeBase = ChildNode & Node etc. makes sense to me.

Sorry, I meant rename ChildNode to ChildNodeBase, re-declare ChildNode as

interface ChildNode extends ChildNodeBase, Node {}

and swap all the uses of ChildNodeBase & Node to ChildNode.

Restoring the previous ChildNode is also an option.

To be honest, I'm leaning the strongest on this one. We can always make that other change sooner, but I am apprehensive about shipping the original break.

@saschanaz
Copy link
Contributor

👍 from me for that idea.

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Aug 3, 2021

Is there an easy place to fix this? I can see overrideTypes.jsonc for the references, but it's unclear to me where to make the change for the declarations of ChildNode and ParentNode.

@saschanaz
Copy link
Contributor

You can insert the following somewhere below "mixins":

"ChildNode": {
  "name": "ChildNodeBase"
}

And then somewhere below "interfaces":

"ChildNode": {
  "name": "ChildNode",
  "implements": ["ChildNodeBase", "Node"]
}

@DanielRosenwasser
Copy link
Member Author

Sorry, I meant to revert the change and go back to the original definitions of ChildNode - but that was still helpful. I'll have a PR soon.

@saschanaz
Copy link
Contributor

saschanaz commented Aug 3, 2021

That should implement your idea #1087 (comment), or did you change your mind?

@DanielRosenwasser
Copy link
Member Author

I wasn't sure whether it was worth it to define the new *Base types, so I've left them out for now. I figured that if it's really broadly needed, we could introduce them in 4.5.

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 a pull request may close this issue.

2 participants