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

Adjust .parentElement type to include SVGElement #1198

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Andarist
Copy link
Contributor

In reality, this also includes | null but this has been removed in 2cd8ad3 so I guess you want to omit that for pragmatic reasons.

Originally this was typed as Element | null (and that probably was the most "correct" type) but that was changed to HTMLELement | null here: #560 . However, SVGElement can appear here in a lot of scenarios.

An HTML element can be a parent of SVG (the simplest <div><svg/></div>), SVG element can be a parent of another SVG element (<svg><rect/></svg>) but also the SVG element can be a parent of an HTML element (<svg><foreignObject><div></div></foreignObject></svg>)

@github-actions
Copy link
Contributor

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

@HolgerJeromin
Copy link
Contributor

Build has failed because you missed to include the generated files.

But did you considered the feedback from #1151 ?

@Andarist
Copy link
Contributor Author

Sorry, I've missed that discussion. I see the points raised there but having to cast this through unknown/any first and then to SVGElement doesn't seem like a good DX. I understand that people might deal with SVG less often than with HTML but there should be a clear way of working with SVGs too. Not everyone might figure out how to "properly" cast this as it's not intuitive.

It also seems that there is no good list of "expectations" about the DOM types. A lot of this is gray area and the APIs don't provide full type safety. It would be great if things like this would be documented somewhere.

@orta
Copy link
Contributor

orta commented Nov 15, 2021

Happy to have a FAQ of some sort, maybe this could be the first one?

@Andarist
Copy link
Contributor Author

@orta where such a FAQ should live in? this repo? main TS docs?

@orta
Copy link
Contributor

orta commented Nov 15, 2021

In here I think, TypeScript's FAQ is already very very long

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.

3 participants