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

Add isNotDomNode check prefer-node-append #512

Merged
merged 12 commits into from
Feb 17, 2020

Conversation

fisker
Copy link
Collaborator

@fisker fisker commented Feb 3, 2020

Attempt to implement a function notNode to check a AST node impossible type of Node(DOM), so we can use on our prefer-modern-dom-apis, prefer-node-append, prefer-node-remove prevent accidentally wrong fix on some case.

Eg:

foo.removeChild(0)
// -------------^ this is impossible to be a `Node`

I've add some, I'm sure can't be Node

[
	'ArrayExpression', // this has to be a array
	'ArrowFunctionExpression', // this has to be a function
	'ClassExpression', // this has to be a function(class)
	'FunctionExpression', // this has to be a function(class)
	'Literal' // this can't be a Node
	'ObjectExpression', // this has to be a object
	// This could be a `Node` in arguments, like `call(...[node])`, but we are ignoring it
	'SpreadElement', // REMOVED for now, it can't be a callee and we always ignore this as argument
	'TemplateLiteral', // this has to be a string
]

//cc @brettz9 WDYT?

@fisker fisker force-pushed the util-not-node branch 2 times, most recently from 1330de5 to 6e32af0 Compare February 3, 2020 08:22
@brettz9
Copy link
Contributor

brettz9 commented Feb 6, 2020

LGTM... However, in cases where one polyfills the DOM (e.g., in my dominum project), an ObjectExpession might fit the bill as a Node.

@fisker
Copy link
Collaborator Author

fisker commented Feb 6, 2020

Can you explain what use case exactly.

I can't understand, how to make a Node with ObjectExpession

@brettz9
Copy link
Contributor

brettz9 commented Feb 6, 2020

One can polyfill DOM for the sake of such as server-side methods which build DOM trees, and allow one to use libraries with familiar semantics such as jQuery which understand the DOM, but only use the DOM to ultimately serialize it back to a string.

For example, we might have:

const div = {
  nodeName: 'div',
  nodeType: 1,
  append () {/**/},
  childNodes: [
    {nodeType: 3, nodeValue: 'Hello'} // A text node
  ],
  get innerHTML () {/**/}
};

// Adding the string result, e.g., as part of a Node server response
resp.end(
  div.appendChild({
    nodeType: 1,
    nodeName: 'b',
    textContent: 'world'
  }).parentElement.innerHTML
);

The above is just a simplified example, and probably there wouldn't be many people passing in object literals.

Such objects would usually be generated by a library, e.g., jQuery (since such libraries as jQuery are not typically doing checks for native code, e.g., checking whether document.createElement('br').constructor.toString() gives function HTMLBRElement() { [native code] }, but merely calling or checking appendChild, innerHTML, etc., all of which can be polyfilled on regular objects), so that one can have something like this on the server:

const parentTemplate = '<div>Hello</div>';
const childTemplate = '<b>world</b>';

resp.end(
  $(parentTemplate).append(childTemplate).html()
);

This example may not seem particularly useful, but just mentioning there are cases where one may polyfill the DOM, and these may be implemented as objects.

@fisker
Copy link
Collaborator Author

fisker commented Feb 6, 2020

Ya, I got it, a fake Node, but I don't think we should consider this case.

@brettz9
Copy link
Contributor

brettz9 commented Feb 6, 2020

Alrightee...

@sindresorhus
Copy link
Owner

I think in such obscure cases, you can use an ESLint disable comment.

@fisker
Copy link
Collaborator Author

fisker commented Feb 14, 2020

@sindresorhus What I'm going to do is ignore that case, so he can't use disable comment.

I mean div.appendChild({ nodeType: 1, nodeName: 'b', textContent: 'world' }). this should not report, this cant be a real Node

@fisker fisker changed the title [WIP] Add notNode to check impossible type of Node Add isNotDomNode check prefer-node-append Feb 16, 2020
@fisker
Copy link
Collaborator Author

fisker commented Feb 16, 2020

I took prefer-node-append rule as a start,

the selector is so strict now

CallExpression
[callee.type="MemberExpression"]
[callee.computed=false]
[callee.property.type="Identifier"]
[callee.property.name="appendChild"]
[arguments.length=1]
[arguments.0.type!="SpreadElement"]
[callee.object.type!="ArrayExpression"]
[callee.object.type!="ArrowFunctionExpression"]
[callee.object.type!="ClassExpression"]
[callee.object.type!="FunctionExpression"]
[callee.object.type!="Literal"]
[callee.object.type!="ObjectExpression"]
[callee.object.type!="TemplateLiteral"]
:not(
	[callee.object.type="Identifier"]
	[callee.object.name="undefined"]
)
[arguments.0.type!="ArrayExpression"]
[arguments.0.type!="ArrowFunctionExpression"]
[arguments.0.type!="ClassExpression"]
[arguments.0.type!="FunctionExpression"]
[arguments.0.type!="Literal"]
[arguments.0.type!="ObjectExpression"]
[arguments.0.type!="TemplateLiteral"]
:not(
	[arguments.0.type="Identifier"]
	[arguments.0.name="undefined"]
)

So we are not reporting following anymore

[].appendChild(foo);

foo.appendChild('not a dom node');

// and many others

@brettz9 @sindresorhus

@sindresorhus sindresorhus merged commit 9bc218e into sindresorhus:master Feb 17, 2020
@fisker fisker deleted the util-not-node branch February 17, 2020 16:13
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