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

#1261: NodeListOf#item #1268

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

danielrentz
Copy link

fixes #1261
NodeListOf#item() misses null in return type

NodeListOf#item() misses `null` in return type
@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

Thanks for helping.
You manipulated a generated file.
The correct place would be overridingTypes.jsonc

@danielrentz
Copy link
Author

Oh I see, thanks for the pointer. Will take care.

@ghost
Copy link

ghost commented Feb 22, 2022

CLA assistant check
All CLA requirements met.

@saschanaz
Copy link
Contributor

I think this is intentionally done to follow the current array indexing behavior. Also pinging @orta

@danielrentz
Copy link
Author

danielrentz commented Feb 25, 2022

I understand that this will not be changed for array index due to the generic behaviour of TS. But it should be done "right" for explicit methods. At least it should be consistent across the interfaces NodeList and NodeListOf. A subtle difference is that array index will return undefined while item returns null.

BTW, how will the new method Array#at behave in TS?

@orta
Copy link
Contributor

orta commented Feb 26, 2022

Hrm, yeah, looks like I was a bit gung ho in #1261

Yeah, .at will/should/does (I've not checked, but that's what we agreed on) have the undefined added in the types but we shouldn't really be switching such a popular DOM API I think (just like we didn't do it to arrays) - those sort of changes aren't really possible at the ecosystem size TS is at nowdays

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.

NodeListOf#item misses return type "null"
4 participants