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
prefer-modern-dom-apis: Only fix when expression is not used #503
prefer-modern-dom-apis: Only fix when expression is not used #503
Conversation
Thanks for fixing this rule. I think we still missing many cases on that parent type check, eg:
`${a.insertBefore(b, c)}` // maybe no one do this way, but still possible
new Dom(a.insertBefore(b, c))
+a.insertBefore(b, c) Not sure can we do opposite way? only allow certain types, or can we check all possible node types, and add more. |
Is this all possible types? https://esprima.readthedocs.io/en/latest/syntax-tree-format.html |
6c8dab0
to
0e4b513
Compare
Ah, good to know re: other parent types. (The PR is already covering With ESLint's parser being Espree, I wouldn't be sure that would cover everything, though it does seem to be a project goal bring fundamental compatibility with Esprima. However, beyond this, other parsers are being used with eslint, including for TypeScript, so I wouldn't be surprised if there are even more. As far as inverting the check, the one parent type in use in the tests which isn't caught here is |
We still not cover all possible cases, like /cc @sindresorhus WDYT? |
What do you mean exactly? |
Yeah, if I may answer, that is correct. As unlikely as such cases may be, there are other types of places in JS we are not currently addressing such as I think fisker and I may agree that only allowing fixers when the parent is a IMO, without going through each type (and that would require looking at TypeScript AST as well, to support users applying such a parser), in either approach, we could be having false negatives or false positives, so I think it would be better to go with the safer approach of only allowing the fixer for |
👍 |
…ere node return values are expected safe not to be used. For `prefer-modern-dom-apis`, the change will avoid fixers in more cases than just `VariableDeclarator` and `AssignmentExpression` parent types (as previously); will now avoid whenever parent is not an `ExpressionStatement` since many other parents, e.g., `ArrayExpression`, `IfStatement`, `'MemberExpression`, `Property`, `ReturnStatement`, etc. can use a return value. For `prefer-node-*` rules, rather than blacklisting an albeit more extensive set of parents, will also switch to a whitelist, potentially avoiding fixers on other contexts not previously avoided, e.g., `AwaitExpression`. The previous avoidance by `prefer-node-*` of fixers with `ExpressionStatement` grandparents is not currently required. Also: 1. provides minor optimization to avoid declaring fixer with value when not needed. 2. fixes a test which made a fix which would break because of an inability of the replacement API to return a value.
0e4b513
to
ff4a6ca
Compare
I've pushed a fix that only allows a fixer for |
Should we change |
I think the result should be the same, but we can invert the naming (though maybe |
Great, let's do this. |
I've updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, thanks.
Co-Authored-By: fisker Cheung <lionkay@gmail.com>
87d8431
to
a5d5d53
Compare
prefer-modern-dom-apis
): Ignore other cases where node return value is used
Providing a fix related to this comment.
fix(prefer-modern-dom-apis
): Ignore other cases where node return value is used- Where parent is anArrayExpression
,IfStatement
,MemberExpression
,Property
,ReturnStatement
Fixes a test which made a fix which would break because of an inability of the replacement API to return a value.Also provides minor optimization to avoid declaring fixer with value when not needed.Also applies (and for
prefer-node-*
rules, switches from) checkingExpressionStatement
in favor of specific parents (BinaryExpression
,CallExpression
,ConditionalExpression
,LogicalExpression
,UnaryExpression
), since in theory, the last part ofparentNode.insertBefore(alfa, beta).insertBefore(charlie, delta);
could be checked if the rule were changed to allow such parent expressions rather than requiring a parent member to be a name only (i.e., fixingparentNode
inparent.insertBefore()
but ignoringsome.expression().insertBefore()
) since technically such a replacement could be made. But allowing it to be made would allow additional refactoring and potentially reporting long errors, e.g., "Prefer some.very.very.long.expression().before() over some.very.very.long.expression().insertBefore", and I'm not sure it is worth the trouble to support this strange pattern. But I did think it at least more logically clear to check the parents.fix(
prefer-modern-dom-apis
): Switch to whitelist for determining where node return values are expected safe not to be used.For
prefer-modern-dom-apis
, the change will avoid fixers in more cases than justVariableDeclarator
andAssignmentExpression
parent types (as previously); will now avoid whenever parent is not anExpressionStatement
since many other parents, e.g.,ArrayExpression
,IfStatement
,'MemberExpression
,Property
,ReturnStatement
, etc. can use a return value.For
prefer-node-*
rules, rather than blacklisting an albeit more extensive set of parents, will also switch to a whitelist, potentially avoiding fixers on other contexts not previously avoided, e.g.,AwaitExpression
.The previous avoidance by
prefer-node-*
of fixers withExpressionStatement
grandparents is not currently required.Also:
isValueNotUsable
overisValueUsed