Add `prefer-modern-dom-apis` rule #362

mcmxcdev · 2019-09-09T07:43:32Z

Fixes #344.

The rule can be tested in this gist here: https://astexplorer.net/#/gist/338d492213d95b40122046fb29abd1bc/bb5030be4689f651524402efad8d24d2b310d588

The links in the description for prepend() and append() from MDN are returning 404 by the way, they should be updated.

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

#344: Rule proposal: prefer-modern-dom-apis

IssueHunt has been backed by the following sponsors. Become a sponsor

Signed-off-by: mhatvan <markus_hatvan@aon.at>

readme.md

MrHen · 2019-09-09T13:02:13Z

rules/prefer-modern-dom-apis.js

+		}
+
+		return null;
+	};


Move this helper function out of checkForReplaceChildOrInsertBefore. There may be a suitable util here: https://github.com/sindresorhus/eslint-plugin-unicorn/tree/master/rules/utils. Perhaps is-method-named?

Also, args is not a very good argument name. Perhaps nodeArgument?

is-method-named would not help me, since i cannot say what the argument name will be.
There are multiple other rules with const args = node.arguments; and their own scoped helper functions, e.g: prefer-node-remove in the code already, but I am also fine with renaming it ;)

rules/prefer-modern-dom-apis.js

MrHen · 2019-09-09T13:07:34Z

rules/prefer-modern-dom-apis.js

+		}
+
+		return null;
+	};


Same comment as before. This should be moved to the parent scope.

rules/prefer-modern-dom-apis.js

MrHen · 2019-09-09T13:09:30Z

rules/prefer-modern-dom-apis.js

+		CallExpression(node) {
+			if (
+				node.callee.type === 'MemberExpression' &&
+				node.arguments.length === 2


You can include these checks in the selector pattern for the rule. Not required for the PR but in case you hadn't seen it yet, here are the docs: https://eslint.org/docs/developer-guide/selectors

test/prefer-modern-dom-apis.js

added additional test cases Signed-off-by: mhatvan <markus_hatvan@aon.at>

mcmxcdev · 2019-09-12T21:28:52Z

@MrHen I pushed some code adaptions in the newest commit.

mcmxcdev · 2019-10-15T22:33:54Z

@MrHen can you take another look at the changes you requested? I implemented your recommendations a while ago.

MrHen · 2019-10-16T02:41:17Z

@mhatvan Sorry, I'm actually in the middle of changing employers right now and am transitioning a ton of stuff across teams. I probably won't get a chance to look at it in the next couple weeks.

readme.md

index.js

rules/prefer-modern-dom-apis.js

fisker · 2019-10-16T03:42:42Z

since childNode.replaceWith() and Node.replaceChild() returns different value

also ChildNode.before()

ChildNode.before()
Node.insertBefore()
Element.insertAdjacentElement()

so, maybe you want do a similar check like #321

…n into feat/344-prefer-modern-dom-apis

* Change getDocsUrl to getDocumentationUrl * Change formatting of plugins property in index.js Signed-off-by: mhatvan <markus_hatvan@aon.at>

…rule Signed-off-by: mhatvan <markus_hatvan@aon.at>

mcmxcdev · 2019-10-21T20:17:43Z

@fisker I pushed some code adaptions in the newest commit.

fisker · 2019-10-22T01:26:55Z

what about this?

since childNode.replaceWith() and Node.replaceChild() returns different value

also ChildNode.before()

ChildNode.before()
Node.insertBefore()
Element.insertAdjacentElement()

so, maybe you want do a similar check like #321

mcmxcdev · 2019-10-22T05:29:17Z

so, maybe you want do a similar check like #321

you linked the complete PR without specifying what you exactly want me to change, I need more details here.

fisker · 2019-10-29T02:11:07Z

you linked the complete PR without specifying what you exactly want me to change, I need more details here.

@mhatvan I mean some methods returns different result, so we need check if the return value is used.

docs/rules/prefer-modern-dom-apis.md

readme.md

* change fenced code block language in readme.md from bash to console Signed-off-by: mhatvan <markus_hatvan@aon.at>

mcmxcdev · 2019-11-12T22:55:39Z

@sindresorhus I implemented the changes you requested, can we get this merged now?

I still don't understand @fisker feedback:

childNode.replaceWith() and Node.replaceChild() returns different value

I implemented it according to the examples in MDN and they return the expected result.

sindresorhus · 2019-11-14T05:52:57Z

@mhatvan Node.replaceChild() returns a value. If the user actually uses this value, we cannot safely auto-fix with with ChildNode.replaceWith() as it doesn't return any value. Can you handle this and add a test for that?

…n into feat/344-prefer-modern-dom-apis

…Before() and .insertAdjacentElement() methods * add additional test cases for .replaceWith(), .insertBefore() and .insertAdjacentElement() methods Signed-off-by: mhatvan <markus_hatvan@aon.at>

mcmxcdev · 2019-12-08T12:09:39Z

@sindresorhus Sorry, I missed that you already left a comment a while ago. I pushed another commit which implements the requested changes and added additional tests.

fisker · 2019-12-08T17:28:56Z

@mhatvan I think we should still report an error, just can't fix

sindresorhus · 2019-12-08T18:51:54Z

@mhatvan I think we should still report an error, just can't fix

👍

… methods to be reported but no autofix option when part of variable assignment * update test cases to verify changed rule logic Signed-off-by: mhatvan <markus_hatvan@aon.at>

mcmxcdev · 2019-12-22T15:24:13Z

@sindresorhus @fisker feedback implemented, I pushed another commit ;)

sindresorhus · 2019-12-22T17:27:38Z

Looks good. Thanks for contributing 👌

fregante · 2019-12-22T22:30:09Z

Awesome!

fisker · 2019-12-23T02:40:01Z

rules/prefer-modern-dom-apis.js

+const isPartOfVariableAssignment = nodeParentType => {
+	if (nodeParentType === 'VariableDeclarator' || nodeParentType === 'AssignmentExpression') {
+		return true;
+	}
+
+	return false;
+};


I don't think this is enough,

simple one:

console.log(referenceNode.insertAdjacentElement("beforebegin", newNode));

Add prefer-modern-dom-apis rule (#344)

8dc3de2

Signed-off-by: mhatvan <markus_hatvan@aon.at>

MrHen suggested changes Sep 9, 2019

View reviewed changes

code adaptions according to requested changes

4a49fe5

added additional test cases Signed-off-by: mhatvan <markus_hatvan@aon.at>

MrHen self-requested a review September 13, 2019 14:33

MrHen self-assigned this Sep 13, 2019

MrHen removed their assignment Oct 16, 2019

fisker requested changes Oct 16, 2019

View reviewed changes

readme.md Show resolved Hide resolved

readme.md Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

fisker requested changes Oct 16, 2019

View reviewed changes

rules/prefer-modern-dom-apis.js Outdated Show resolved Hide resolved

mhatvan added 3 commits October 21, 2019 21:50

Merge branch 'master' of github.com:sindresorhus/eslint-plugin-unicor…

26f6712

…n into feat/344-prefer-modern-dom-apis

* Restore blank lines in readme.md

150ea81

* Change getDocsUrl to getDocumentationUrl * Change formatting of plugins property in index.js Signed-off-by: mhatvan <markus_hatvan@aon.at>

Change variable naming to conform with prevent-abbreviations linting …

c036011

…rule Signed-off-by: mhatvan <markus_hatvan@aon.at>

MrHen removed their request for review October 29, 2019 01:35

sindresorhus requested changes Nov 12, 2019

View reviewed changes

docs/rules/prefer-modern-dom-apis.md Outdated Show resolved Hide resolved

readme.md Outdated Show resolved Hide resolved

* linkify methods to MDN documentation in prefer-modern-dom-apis.md

52812d3

* change fenced code block language in readme.md from bash to console Signed-off-by: mhatvan <markus_hatvan@aon.at>

sindresorhus and others added 5 commits November 14, 2019 12:54

Update prefer-modern-dom-apis.md

9bbcf0e

Update prefer-modern-dom-apis.md

f81eeec

Update readme.md

dbf05fe

Merge branch 'master' of github.com:sindresorhus/eslint-plugin-unicor…

046d69e

…n into feat/344-prefer-modern-dom-apis

* add logic to ignore variable assignment for .replaceWith(), .insert…

b8a3d4a

…Before() and .insertAdjacentElement() methods * add additional test cases for .replaceWith(), .insertBefore() and .insertAdjacentElement() methods Signed-off-by: mhatvan <markus_hatvan@aon.at>

* change .replaceWith(), .insertBefore() and .insertAdjacentElement()…

3bde7f0

… methods to be reported but no autofix option when part of variable assignment * update test cases to verify changed rule logic Signed-off-by: mhatvan <markus_hatvan@aon.at>

sindresorhus added 2 commits December 22, 2019 18:02

Update prefer-modern-dom-apis.js

d513baf

Update prefer-modern-dom-apis.js

98518d0

sindresorhus merged commit 44d14b9 into sindresorhus:master Dec 22, 2019

fisker reviewed Dec 23, 2019

View reviewed changes

fisker mentioned this pull request Jan 20, 2020

prefer-node-remove: Only fix when expression is not used #498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `prefer-modern-dom-apis` rule #362

Add `prefer-modern-dom-apis` rule #362

mcmxcdev commented Sep 9, 2019 •

edited

MrHen Sep 9, 2019

mcmxcdev Sep 9, 2019

MrHen Sep 9, 2019

MrHen Sep 9, 2019 •

edited

mcmxcdev commented Sep 12, 2019

mcmxcdev commented Oct 15, 2019

MrHen commented Oct 16, 2019 •

edited

fisker commented Oct 16, 2019

mcmxcdev commented Oct 21, 2019

fisker commented Oct 22, 2019

mcmxcdev commented Oct 22, 2019 •

edited

fisker commented Oct 29, 2019

mcmxcdev commented Nov 12, 2019

sindresorhus commented Nov 14, 2019

mcmxcdev commented Dec 8, 2019

fisker commented Dec 8, 2019

sindresorhus commented Dec 8, 2019

mcmxcdev commented Dec 22, 2019

sindresorhus commented Dec 22, 2019

fregante commented Dec 22, 2019

fisker Dec 23, 2019 •

edited

Add prefer-modern-dom-apis rule #362

Add prefer-modern-dom-apis rule #362

Conversation

mcmxcdev commented Sep 9, 2019 • edited

Referenced issues

MrHen Sep 9, 2019

Choose a reason for hiding this comment

mcmxcdev Sep 9, 2019

Choose a reason for hiding this comment

MrHen Sep 9, 2019

Choose a reason for hiding this comment

MrHen Sep 9, 2019 • edited

Choose a reason for hiding this comment

mcmxcdev commented Sep 12, 2019

mcmxcdev commented Oct 15, 2019

MrHen commented Oct 16, 2019 • edited

fisker commented Oct 16, 2019

mcmxcdev commented Oct 21, 2019

fisker commented Oct 22, 2019

mcmxcdev commented Oct 22, 2019 • edited

fisker commented Oct 29, 2019

mcmxcdev commented Nov 12, 2019

sindresorhus commented Nov 14, 2019

mcmxcdev commented Dec 8, 2019

fisker commented Dec 8, 2019

sindresorhus commented Dec 8, 2019

mcmxcdev commented Dec 22, 2019

sindresorhus commented Dec 22, 2019

fregante commented Dec 22, 2019

fisker Dec 23, 2019 • edited

Choose a reason for hiding this comment

Add `prefer-modern-dom-apis` rule #362

Add `prefer-modern-dom-apis` rule #362

mcmxcdev commented Sep 9, 2019 •

edited

MrHen Sep 9, 2019 •

edited

MrHen commented Oct 16, 2019 •

edited

mcmxcdev commented Oct 22, 2019 •

edited

fisker Dec 23, 2019 •

edited