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

Standardize DOM node type checking #3607

Open
gibson042 opened this issue Apr 2, 2017 · 3 comments
Open

Standardize DOM node type checking #3607

gibson042 opened this issue Apr 2, 2017 · 3 comments

Comments

@gibson042
Copy link
Member

Description

Our style guide says that we identify DOM elements with (Boolean) object.nodeType, but that's not exactly true. We actually use it to identify DOM nodes, but even setting aside semantics, there is a case where an object with a truthy nodeType property can avoid the DOM node path: setting data.

This means that .data can succeed or fail based on some rather trivial differences, which makes me uncomfortable. We should have a single "is DOM node" test, applied consistently everywhere. nodeType will always have certain characteristics on real DOM nodes (existence, "number" type, integer, positive), but we can use any subset thereof—I'm leaning towards typeof obj.nodeType === "number" or obj && "nodeType" in obj, because any given kind of input seems unlikely to change classification. And whatever we pick, the style guide will also need an update.

A macro would be really nice, because it seems silly not to inline such trivial code, but I think we can continue to live without something like sweet.js for now.

Link to test case

https://jsbin.com/kafudoseno/edit?html,js,console

@gibson042 gibson042 added the Core label Apr 2, 2017
@timmywil
Copy link
Member

timmywil commented Apr 3, 2017

This seems like bikeshedding to me. I've never had the need to have a nodeType property on an object unless I was mocking something in tests. That said, if the change is small, I don't mind.

@timmywil timmywil added this to the Future milestone Apr 10, 2017
@tumulr
Copy link

tumulr commented Dec 12, 2017

If we just only check for the nodeType to be number, it will allow all the other types of nodes too right(Attribute node -> nodeType = 2). Will that not cause an issue?

@gibson042 What are your thoughts on it?

@gibson042
Copy link
Member Author

I'm not talking about element tests (which are always nodeType === 1), but about node tests, for which we have no standard (and in practice use boolean-cast nodeType almost exclusively, the main exception being acceptData).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants