1508992 - Enable ESLint for dom/plugins

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Mark Banner (:standard8)]

As part of rolling out ESLint across the tree, we should enable it for dom/plugins.

Comment 1

[hudaa]

Hi, I would like to take this issue :) Thank you!

Comment 2

[hudaa]

Hi Mark, I know I have realized this a little too late but I have noticed that in the .eslintignore file there is two directories that include Dom/plugins.
Should I assume that I will be working on both? (I have already started on one of them but just want to confirm) thank you!

Comment 3

[Mark Banner (:standard8)]

Hi hudaa, it would be nice to do both, but if you're pushed for time, just doing one is fine. If you've already done dom/plugins/test/mochitest then I think there's only a few things to fix for dom/plugins/test/unit/

Comment 4

[hudaa]

I will definitely complete both :)
I'm just struggling with a few errors that I can't seem to find any help with online.
One of them is:
'Function testObject has a complexity of 35'
^ It's associated with a pretty long function(Please let me know if you would like me to post it here) and I can't seem to understand what I need to fix here for the error to go away.

Another one Im having an issue with is: 
'Expected to return a value at the end of arrow function'

This is the code sample, I understand that it's expecting a return statement at the end but a lot of examples I see online talk about 'map' but I'm a bit confused as to what is expected to be return here

'SimpleTest.registerCleanupFunction(() => {
      if (this.document.mozFullScreenElement) {
        let fullScreenChange = promiseFullScreenChange();
        this.document.mozCancelFullScreen();
        return fullScreenChange;
      }
    });'


Sorry for asking so many questions and I appreciate anything help I can receive. Thank you!

Comment 5

[Mark Banner (:standard8)]

(In reply to hudaa from comment #4)
> One of them is:
> 'Function testObject has a complexity of 35'
> ^ It's associated with a pretty long function(Please let me know if you
> would like me to post it here) and I can't seem to understand what I need to
> fix here for the error to go away.

This is probably with the pre-existing functions, unfortunately some have got a bit too long and complex. In this case we tend to just disable the rule for when we're enabling ESlint on new code, so you can add a comment like:

```
// eslint-disable-next-line complexity
function complexFunction() {
```

> Another one Im having an issue with is: 
> 'Expected to return a value at the end of arrow function'
> 
> This is the code sample, I understand that it's expecting a return statement
> at the end but a lot of examples I see online talk about 'map' but I'm a bit
> confused as to what is expected to be return here
> 
> 'SimpleTest.registerCleanupFunction(() => {
>       if (this.document.mozFullScreenElement) {
>         let fullScreenChange = promiseFullScreenChange();
>         this.document.mozCancelFullScreen();
>         return fullScreenChange;
>       }
>     });'

So fullScreenChange is a Promise, and it is complaining that for the `!this.document.mozFullScreenElement` case that there's no return value.

Probably the easiest thing is to add another line after the if section that is just `return Promise.resolve();`. The alternative would be to switch this to an async function and `await fullScreenChange` rather than return it, but I think that's a bit more than what we need here.

> Sorry for asking so many questions and I appreciate anything help I can
> receive. Thank you!

No problem.

Comment 6

[hudaa]

Thank you so much Mark!

Comment 7

[hudaa]

Attachment: Bug 1508992 - Enable ESLint for dom/plugins/test/mochitest/ (automatic changes)

Comment 8

[hudaa]

Attachment: Bug 1508992 - Enable ESLint for dom/plugins/test/mochitest/ (automatic changes)

Comment 9

[Mark Banner (:standard8)]

Hi hudaa, I'm not quite sure what's happened here, but these are both automatic changes.

Did you use moz-phab to submit them or arc directly? If you used arc, it might be best to swap to moz-phab as that handles multiple commits better. To do that, it is probably best to abandon these revisions (there's an action menu just above the comment box at the bottom), then, edit the two commits you've already made (with `hg histedit`) to remove anything apart from the descriptions. Then use moz-phab to submit.

Comment 10

[hudaa]

I apologize for that, I have realized the mistake and I'm currently trying to fix them up.
TO be honest, I'm still having a hard time getting moz-phab to setup and I'm currently doing some research to see what I'm doing wrong and why they are not going through. Will get this done asap.

Comment 11

[hudaa]

Attachment: Bug 1508992 - Enable ESLint for dom/plugins/test/mochitest/ (manual changes)

Depends on D13719

Comment 12

[hudaa]

Hi Mark, I have followed your suggestion and finally got moz-phab working (I think)
I do see automatic and manual changes attachments now but not as comments above so I'm not sure if that's okay.
Please let me know if something is wrong or if I need to redo something.
Thank you for your patient!

Comment 13

[Mark Banner (:standard8)]

Hi hudaa, https://phabricator.services.mozilla.com/D13769 seems wrong - it seems to be a very large diff that doesn't really relate to the proper item.

Can you take another look. Using moz-phab you should just be able to have your two commits applied, then do `moz-phab submit` without any extra arguments.

Comment 14

[hudaa]

Hi Mark,
the problem Im having is, when I run 'moz-phab submit' I get a message that the command moz-phab is not found.
I have a MacOS so according to the instruction here
https://github.com/mozilla-conduit/review/blob/master/README.md

I'm supposed to download the script and paste it in the path which I have done.
I assumed we needed to run the scripts too but everytime I try to run it, I also get an error message that there's too few arguments (and the options i get is 'submit' which is supposed to commit to phabricator) and that's how I got the commits.

I'm just confused as to how I should actually get moz-phab set up properly.
Thank you!

Comment 15

[hudaa]

Attachment: Bug 1508992 - Enable ESLint for dom/plugins/test/mochitest/ (manual changes)

Depends on D13769

Comment 16

[Shivam Singhal [ :championshuttler ]]

Looks like its been a while for a update here. Can i work on it @Mark?

Most probably I will going to fix dom/plugins/test/mochitest/** then will attach patches for the next part :)

Thanks

Comment 17

[Shivam Singhal [ :championshuttler ]]

Attachment: Bug 1508992 - Enable ESLint for dom/plugins/test/mochitest (Automatic changes). r=standard8

Comment 18

[Shivam Singhal [ :championshuttler ]]

Attachment: Bug 1508992 - Enable ESLint for dom/plugins/test/mochitest (Manual changes). r=standard8

Depends on D20093

Comment 19

[Mark Banner (:standard8)]

(In reply to Shivam Singhal [ :championshuttler ] from comment #16)

Looks like its been a while for a update here. Can i work on it @Mark?

In future I'd prefer a little more time between asking and working/attaching on patches. I don't work weekends, and some of these bugs I've been working with the people outside of bugzilla as it has been easier for discussion/sharing. I do generally keep track of these but I haven't been ready to resolve them all just yet.

Also where there's existing patches I'll sometimes prefer they are re-used.

In this case I haven't heard from hudaa for a while, so I assume they've gone onto other things, so I think we're ok to continue which you.

Comment 20

[Shivam Singhal [ :championshuttler ]]

Now the directory dom/plugins/test/unit is remained, will attach the patches soon.

Comment 21

[Mark Banner (:standard8)]

Removing checkin-needed, as try still seems unhappy :-( https://treeherder.mozilla.org/#/jobs?repo=try&revision=60cbeb71e2ed59a3b9ea252aba1f1e424c6daea8&selectedJob=230774572

Comment 22

[Mark Banner (:standard8)]

I can't figure this out - it is working fine locally. So I'm doing a fresh try build to see if it was just a dodgy artifact build or something:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=584ffa0777adebb23c7000917565ac5b6ca46968

Comment 23

[Mark Banner (:standard8)]

We worked it out and the latest try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6dd947b775404d21f7f18f873e4da99dddb3538

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c1caae29d7c
Enable ESLint for dom/plugins/test/mochitest (Automatic changes). r=Standard8,qdot
https://hg.mozilla.org/integration/autoland/rev/2993263a193c
Enable ESLint for dom/plugins/test/mochitest (Manual changes). r=Standard8,qdot

Comment 25

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8c1caae29d7c
https://hg.mozilla.org/mozilla-central/rev/2993263a193c

Comment 26

[Shivam Singhal [ :championshuttler ]]

Attachment: Bug 1508992 - Enable ESLint for dom/plugins/test/unit (Automatic Changes). r=standard8

Comment 27

[Shivam Singhal [ :championshuttler ]]

Attachment: Bug 1508992 - Enable ESLint for dom/plugin/test/unit (Manual Changes). r=standard8

Depends on D22365

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/519402d3b189
Enable ESLint for dom/plugins/test/unit (Automatic Changes). r=qdot
https://hg.mozilla.org/integration/autoland/rev/c7009174ac7e
Enable ESLint for dom/plugin/test/unit (Manual Changes). r=qdot

Comment 29

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/519402d3b189
https://hg.mozilla.org/mozilla-central/rev/c7009174ac7e