Add prefer-event-key rule

[ankeetmaini]

Fixes #118

• add fixer code

[ankeetmaini]

I think my approach isn't very good here, trying to first narrow down to addEventListener and then finding all the keyCode bits using SourceCode's getToken API. Because querying under a Node is a little difficult and by doing getTokens I lose the parent-child link. This will make it unusable for destructured elements.

Following up with a better one where in I directly select keyCode identifier and then using context.getAncestors() to assert that they're infact inside an event callback.

[futpib]

One foreseeable gotcha with context.getAncestors() is that a .keyCode that is syntactically inside an addEventListener callback is not exactly the same as a .keyCode on the callback's argument.

const something = undefined;
element.addEventListener('keyup', event => {
  console.log(something.keyCode); // This `.keyCode` is "inside an event callback"
});

I think this can be correctly solved by targeting the callback function in the visitor, then getting it's scope and checking all the references to the first argument to see if they are .keyCode.

[ankeetmaini]

I'll have the CallbackExpression in the ancestors and was in process to do the same thing! I'll double back shortly on how it goes :)

Also I assume we only want to target keyup and keydown events. I'll add that check as well.

[futpib]

Also I assume we only want to target keyup and keydown events. I'll add that check as well.

We might as well target all events, since accessing keyCode (and others) on non-KeyboardEvents makes no sense, as it will always be undefined.

@sindresorhus Any thoughts?

@ankeetmaini If we indeed want to target only KeyboardEvents, there is also keypress.

[sindresorhus]

Hey, thanks for the pull request 🙌 I have gotten a huge response on the IssueHunt bounty issues and I now have a very long backlog of pull requests to review, so it might take some time until I will be able to review this. However, you can help move this along by helping out and reviewing the other submissions. If everyone helps everyone we can manage this 👌.

Some things to look for when reviewing:

• Ensure the rule addition adheres to the instructions.
• There are never too many tests. Try to find both passing and failing cases, especially obscure cases.
• Check the docs for typos and whether something could be more clear.
• Look at the rule code and see if anything could be simplified or done better. If something is unclear in the code, ask the submitter to explain.

[sindresorhus]

We might as well target all events, since accessing keyCode (and others) on non-KeyboardEvents makes no sense, as it will always be undefined.

👍

[ankeetmaini]

@futpib this version shows my current approach. It also takes care of the edge case you suggested. Will poke a bit more before making it final.

[ankeetmaini]

I am happy with the current approach and have added a lot of tests for valid/invalid code. The rule also covers the destructuring cases too. Please take a look :)

The thing that I am not convinced on is the fixing part. How do we fix if the case is like

bar.addEventListener('keyup', e => {
  sendThisToAnOtherModule(e.keyCode)
})

In this even if I try to fix it, it'll break the contract as the the receiving party might not accept a string representation of the key. And in other case like

const keys = {
  'enter': 34
}
bar.addEventListener('keyup', e => {
  if (e.keyCode === keys.enter)  return;
})

I am not sure if we should try to fix as we might step over and break things. Please advise!

[futpib]

I think best we can do is fix only comparisons against literals (like event.keyCode === 27 => event.key === 'Escape')

[ankeetmaini]

Update: @futpib I've added the fixer code which takes care of direct if conditions.

if (e.keyCode === 27) -> if (e.key === 'Escape')

but the case

const {keyCode} = e;
if (keyCode === 27) // do something

needs to be looked into.

[futpib]

@ankeetmaini Would be nice to handle destructurings too, but I think we can give it a pass, as other rules like this (prefer-node-remove (with respect to parentNode/parentElement) and prefer-text-content) don't fix it too.

Unless @sindresorhus disagrees.

[ankeetmaini]

@futpib I'm planning to give fixing-destructured-variables a try over the weekend. Will come back with an update.

[ankeetmaini]

Hi @futpib, this is ready for review now. Also I couldn't figure out a way to fix destructured variables in if conditions.

I tried multiple ways, only one handler for all all keyCode, which etc (currently I target Property for destructured keys) but it threw as it was generating duplicate errors one for the key part and one for value

const {keyCode} = event

Here this generates into two identifiers, where both the key and value of keyCode is same, but the tests couldn't figure out === equality and were failing.

Even if I fix this, when the above variable is referenced again I wasn't able to find out if it is resolved from the destructured variable, as the AST walk function is called separately for them.

I also tried using context.getScope() it returns all the references but I don't get the parent attribute (weird!) and hence wasn't able to assert if it's in an If condition.

[futpib]

@sindresorhus Should this rule be named prefer-key or prefer-event-key, without the "over something else" part, like the other rules, or is the current name fine?

[sindresorhus]

Yes, it should be prefer-event-key.

[futpib]

I also tried using context.getScope() it returns all the references but I don't get the parent attribute (weird!) and hence wasn't able to assert if it's in an If condition.

I guess you were looking for referece.identifier.parent. But fixing destructuring was an optional goal anyway.

[ankeetmaini]

@futpib thanks a lot for a thorough review! I've fixed and updated the implementation as per your comments. Let me know!

[sindresorhus]

Please describe that it's fixable and what is fixable. See the other docs for how to write it.

[sindresorhus]

@ankeetmaini Can you fix the merge conflict and resolve the last feedback?

[ankeetmaini]

@sindresorhus I am so sorry for not getting to it I'll update this shortly. Thanks for updating this PR. Will also address @futpib's comment. :)

[ankeetmaini]

@sindresorhus all requested changes have been pushed!

[sindresorhus]

// @MrHen

[sindresorhus]

Looks good to me now. Thanks for contributing, @ankeetmaini :)

[ankeetmaini]

Thanks so much! Yay!