Fix eslint for new console output

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Developer Tools: Console
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: ntim, Assigned: nchevobbe)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
(Reporter)

Updated

2 years ago
Blocks: 1231957
Priority: -- → P3
(Assignee)

Updated

a year ago
Assignee: nobody → chevobbe.nicolas
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
Sorry the stubs have been modified to to some formatting (and some packets changes too).
Mocha tests are green, TRY is running https://treeherder.mozilla.org/#/jobs?repo=try&revision=8eef8d040a536f62514420a63e4078ccf0ed0f39

Brian if you could have a quick look at this before the end of the week, I'll greatly appreciate :) Thanks !
(Reporter)

Comment 3

a year ago
mozreview-review
Comment on attachment 8817463 [details]
Bug 1317069 - Fix ESLint in new console frontend;

https://reviewboard.mozilla.org/r/97718/#review98092

::: .eslintignore:106
(Diff revision 1)
> -!devtools/client/webconsole/jsterm.js
> -!devtools/client/webconsole/console-commands.js
> +devtools/client/webconsole/console-output.js
> +devtools/client/webconsole/hudservice.js
> +devtools/client/webconsole/utils.js
> +devtools/client/webconsole/webconsole-connection-proxy.js
> +devtools/client/webconsole/webconsole.js
> +devtools/client/webconsole/new-console-output/test/fixtures/stubs/

Can we create ESLint friendly subs? (or at least make them as ESLint friendly as possible and disable some rules for stubs). It bothers me just having the folder listed here :)

::: devtools/client/webconsole/new-console-output/main.js:5
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> - /* global BrowserLoader */
> + /* global BrowserLoader, window */

I don't have a strong preference, but you could set the eslint environment to browser in the config file.

::: devtools/client/webconsole/new-console-output/test/chrome/head.js:3
(Diff revision 1)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
> +/* exported Assert, Task, browserRequire */

a search on dxr tells me that Assert isn't being used in other files: https://dxr.mozilla.org/mozilla-central/search?q=Assert+path%3Adevtools%2Fclient%2Fwebconsole%2F&redirect=false
(Assignee)

Comment 4

a year ago
Thanks Tim :) Do you mind if I put you as a reviewer for this, since you already looked at it ?

> Can we create ESLint friendly subs? (or at least make them as ESLint friendly as possible and disable some rules for stubs). It bothers me just having the folder listed here :)

The stubs are generated, so I don't know if it makes sense to make them eslint-compliant. I'll check how difficult it can be and see.
(Reporter)

Comment 5

a year ago
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #4)
> Thanks Tim :) Do you mind if I put you as a reviewer for this, since you
> already looked at it ?
Sure, go ahead :)

> > Can we create ESLint friendly subs? (or at least make them as ESLint friendly as possible and disable some rules for stubs). It bothers me just having the folder listed here :)
> 
> The stubs are generated, so I don't know if it makes sense to make them
> eslint-compliant. I'll check how difficult it can be and see.
We could do some small tweaks like using 2 space indentation or fixing the missing semis. If it introduces some unnecessary complications to fix a rule (such as max-len), you can just disable it for the file (/* eslint-disable max-len */ or using a eslint config file).
Comment hidden (mozreview-request)
(Reporter)

Comment 7

a year ago
mozreview-review
Comment on attachment 8817463 [details]
Bug 1317069 - Fix ESLint in new console frontend;

https://reviewboard.mozilla.org/r/97718/#review98120

::: devtools/client/webconsole/new-console-output/main.js:6
(Diff revision 2)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>   /* global BrowserLoader */
> + /* exported XPCOMUtils */

DXR tells me that XPCOMUtils isn't used anywhere except browser_webconsole_observer_notifications.js.

The instance in the test should be defined by the test infrastructure, not by this file, so you should be able to safely remove the import in this file. If that's not the case, I'd rather have it defined within browser_webconsole_observer_notifications.js rather than globally.

::: devtools/client/webconsole/new-console-output/test/chrome/head.js:3
(Diff revision 2)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
> +/* exported Assert, Task, browserRequire */

nit: remove Assert from the exported comment as well

::: devtools/client/webconsole/new-console-output/test/components/network-event-message.test.js:62
(Diff revision 2)
> -      expect(wrapper.find("div.message.cm-s-mozilla span.message-body.devtools-monospace").length).toBe(1);
> +      expect(wrapper
> +        .find("div.message.cm-s-mozilla span.message-body.devtools-monospace").length

This formatting is really odd.
Maybe: 
```
let selector = "...";
expect(wrapper.find(selector).length).toBe(1);
```

::: devtools/client/webconsole/new-console-output/test/components/network-event-message.test.js:78
(Diff revision 2)
> -      expect(wrapper.find("div.message.cm-s-mozilla span.message-body.devtools-monospace").length).toBe(1);
> +      expect(wrapper
> +        .find("div.message.cm-s-mozilla span.message-body.devtools-monospace").length
> +      ).toBe(1);

Same here.

::: devtools/client/webconsole/new-console-output/test/fixtures/serviceContainer.js:16
(Diff revision 2)
> +  // eslint-disable-next-line react/display-name
>    createElement: tagName => document.createElement(tagName)

I don't see why this should be needed. Is it possible to remove the comment ?

::: devtools/client/webconsole/new-console-output/test/require-helper.js:42
(Diff revision 2)
>      case "Services.default":
>        return `module.exports = require("devtools/client/webconsole/new-console-output/test/fixtures/Services")`;
>      case "devtools/shared/client/main":
>        return `module.exports = require("devtools/client/webconsole/new-console-output/test/fixtures/ObjectClient")`;
>    }
> +  return undefined;

Maybe something similar to `return path;` could work? If not, I'm fine with undefined.
Attachment #8817463 - Flags: review?(ntim.bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

a year ago
Pushed by chevobbe.nicolas@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d4f99821f915
Fix ESLint in new console frontend; r=ntim

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d4f99821f915
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.