Enable ESLint for dom/plugins

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
6 months ago
2 months ago

People

(Reporter: standard8, Assigned: championshuttler)

Tracking

(Blocks 1 bug)

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

Details

Attachments

(4 attachments, 4 obsolete attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
As part of rolling out ESLint across the tree, we should enable it for dom/plugins.
Priority: -- → P3

Comment 1

6 months ago
Hi, I would like to take this issue :) Thank you!
Assignee: standard8 → huda_wadi

Comment 2

6 months ago
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!
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

6 months ago
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!
(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

6 months ago
Thank you so much Mark!
Attachment #9029368 - Attachment description: Bug 1508992 - Enable ESLint for dom/plugins/test/mochitest/ (automatic changes) r?standard8! → Bug 1508992 - Enable ESLint for dom/plugins/test/mochitest/ (automatic changes)
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.
Flags: needinfo?(huda_wadi)

Comment 10

6 months ago
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.
Flags: needinfo?(huda_wadi)
Attachment #9029368 - Attachment is obsolete: true
Attachment #9029383 - Attachment is obsolete: true
Attachment #9029383 - Attachment is obsolete: false

Comment 12

6 months ago
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!
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.
Flags: needinfo?(huda_wadi)

Comment 14

6 months ago
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!
Flags: needinfo?(huda_wadi)

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

Flags: needinfo?(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.

Assignee: huda_wadi → shivams2799
Flags: needinfo?(standard8)
Attachment #9029383 - Attachment is obsolete: true
Attachment #9029524 - Attachment is obsolete: true
Attachment #9030119 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [seneca-eslint] → checkin-needed

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

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 24

3 months ago
Pushed by mbanner@mozilla.com:
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
Status: NEW → ASSIGNED
Whiteboard: checkin-needed

Comment 28

3 months ago

Pushed by ccoroiu@mozilla.com:
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

Keywords: checkin-needed
Whiteboard: checkin-needed

Comment 29

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.