Closed Bug 1256768 Opened 4 years ago Closed 2 years ago

[ESLint] Fix ESLint errors/warnings in devtools/client/webconsole/console-output.js

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: linclark, Assigned: mkohler, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [btpp-backlog])

Attachments

(1 file, 2 obsolete files)

This is an easy bug for new contributors.

If you haven’t contributed to Firefox before, follow the steps here to set up your environment: https://developer.mozilla.org/en-US/docs/Tools/Contributing#Getting_set_up

Then, automatically configure ESLint to work with the Firefox specific rules by following the instructions here: https://wiki.mozilla.org/DevTools/CodingStandards

Then you can see the issues that need to be fixed by running

> eslint --no-ignore devtools/client/webconsole/console-output.js
Blocks: 1256948
Priority: -- → P3
Whiteboard: [btpp-backlog]
Assignee: nobody → mkohler
Status: NEW → ASSIGNED
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=975d7709e5e5

These look pretty intermittent to me.
Attachment #8744641 - Flags: review?(lclark)
Comment on attachment 8744641 [details] [diff] [review]
0001-Fix-ESLint-Errors-in-console-output.js.patch

Review of attachment 8744641 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! There are some failures in there that aren't already marked as intermittent, so I'd like to see a cleaner try run before committing. For example: 151 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_bug_922212_console_dirxml.js | The ElementNode widget isn't linked to the inspector -

::: devtools/client/webconsole/console-output.js
@@ +2406,5 @@
>      let { preview } = this.objectActor;
>      let { ownProperties, safeGetterValues } = preview;
>      let shown = 0;
>      let getValue = desc => {
> +      let value = "";

The else should be catching anything that hasn't already returned. Can we just remove the `else` wrapper around return `desc.value` so that it's clear that that's the default return?

@@ -3080,5 @@
>        case Ci.nsIDOMNode.DOCUMENT_NODE:
>          this._renderDocumentNode();
>          break;
>        case Ci.nsIDOMNode.ATTRIBUTE_NODE: {
> -        let {preview} = this.objectActor;

So this removes the assignment of preview, but it looks like preview is used below. WHat rule was this hitting?

@@ +3247,5 @@
>      let isAttached = yield this.toolbox.walker.isInDOMTree(this._nodeFront);
>      if (isAttached) {
>        yield this.toolbox.highlighterUtils.highlightNodeFront(this._nodeFront);
>      } else {
> +      throw new Error("node is not attached");

Hm, this actually changes what the code does. While I don't think throwing null is a good thing to do here, changing it might break things. I'm not sure if anything depends on this being how it is. Out of curiosity, which rule is this hitting?
Attachment #8744641 - Flags: review?(lclark) → review-
(In reply to Lin Clark [:linclark] from comment #2)
> ::: devtools/client/webconsole/console-output.js
> @@ +2406,5 @@
> >      let { preview } = this.objectActor;
> >      let { ownProperties, safeGetterValues } = preview;
> >      let shown = 0;
> >      let getValue = desc => {
> > +      let value = "";
> 
> The else should be catching anything that hasn't already returned. Can we
> just remove the `else` wrapper around return `desc.value` so that it's clear
> that that's the default return?

The thing is that we're hitting this rule:

2416:15  error  Unexpected 'else' after 'return'                        no-else-return

We probably want to set desc.value as default and then overwrite it if necessary. Updated my patch to do that.


> @@ -3080,5 @@
> >        case Ci.nsIDOMNode.DOCUMENT_NODE:
> >          this._renderDocumentNode();
> >          break;
> >        case Ci.nsIDOMNode.ATTRIBUTE_NODE: {
> > -        let {preview} = this.objectActor;
> 
> So this removes the assignment of preview, but it looks like preview is used
> below. WHat rule was this hitting?

This was hitting

3026:14  error  'preview' is already declared in the upper scope  no-shadow

but my fix is bad since it's not given that it's the same. We might need to change the rules, I have no idea where the "upper scope" might come from to be honest.


> @@ +3247,5 @@
> >      let isAttached = yield this.toolbox.walker.isInDOMTree(this._nodeFront);
> >      if (isAttached) {
> >        yield this.toolbox.highlighterUtils.highlightNodeFront(this._nodeFront);
> >      } else {
> > +      throw new Error("node is not attached");
> 
> Hm, this actually changes what the code does. While I don't think throwing
> null is a good thing to do here, changing it might break things. I'm not
> sure if anything depends on this being how it is. Out of curiosity, which
> rule is this hitting?


3252:7   error  Expected an object to be thrown                   no-throw-literal

What would you suggest here?



In summary:

/Users/mkohler/development/gecko-dev/devtools/client/webconsole/console-output.js
  3026:14  error  'preview' is already declared in the upper scope  no-shadow
  3252:7   error  Expected an object to be thrown                   no-throw-literal

Patch is updated to latest code changes.


Please also note that I haven't run the tests so far since it's WIP.
Flags: needinfo?(lclark)
Attachment #8744641 - Attachment is obsolete: true
Sorry for the delay.

> 3026:14  error  'preview' is already declared in the upper scope  no-shadow

upper scope would be the scope outside of the switch/case. let uses what's called block scoping.

> 3252:7   error  Expected an object to be thrown                   no-throw-literal
> What would you suggest here?

If we run the tests and it doesn't break anything, then throwing an error (as you have done) is probably the best thing here. The only change is to capitalize/punctuate the error message.
Flags: needinfo?(lclark)
Attachment #8747925 - Attachment is obsolete: true
Comment on attachment 8856211 [details]
Bug 1256768 - Fix ESLint errors/warnings in devtools/client/webconsole/console-output.js

Hello Michael, thanks for working on this !
Lin is not in the team anymore so you can ask either bgrins or I (nchevobbe) for review on the console.
I will review your patch tomorrow morning :)
Attachment #8856211 - Flags: review?(lclark) → review?(nchevobbe)
Comment on attachment 8856211 [details]
Bug 1256768 - Fix ESLint errors/warnings in devtools/client/webconsole/console-output.js

https://reviewboard.mozilla.org/r/128156/#review130786

Overall this looks good, I only have a few nits.
I pushed to TRY with a broader set of tests to make sure there's nothing we missed : https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0c36526f65e697b4659f6a8898e221a40da73b4&selectedJob=90014786
If this is green, feel free to land your patch after fixing the 2 issues in the review.

Thanks

::: devtools/client/webconsole/console-output.js:2294
(Diff revision 1)
> -  _renderObjectProperty: function (key, value, container, needsComma, valueIsText = false)
> -  {
> +  _renderObjectProperty: function (key, value, container, needsComma,
> +                                   valueIsText = false) {

I think we could make this more readable like this :
```
_renderObjectProperty: function (
  key, 
  value, 
  container, 
  needsComma,
  valueIsText = false
) {

```

What do you think ?

::: devtools/client/webconsole/console-output.js:2921
(Diff revision 1)
>    },
>  
> -  render: function ()
> -  {
> +  render: function () {
> +    const {preview} = this.objectActor;
> +
>      switch (this.objectActor.preview.nodeType) {

here we could can now use `preview.nodeType` insteqd of `this.objectActor.preview.nodeType`

::: devtools/client/webconsole/console-output.js:3104
(Diff revision 1)
> -    this._nodeFront = yield this.toolbox.walker.getNodeActorFromObjectActor(this.objectActor.actor);
> +    const walker = this.toolbox.walker;
> +    this._nodeFront = yield walker.getNodeActorFromObjectActor(this.objectActor.actor);

I don't think we need to create a single use new variable here.
What do you think of :
```
this._nodeFront = yield this.toolbox.walker.getNodeActorFromObjectActor(
  this.objectActor.actor);
```
Attachment #8856211 - Flags: review?(nchevobbe)
Comment on attachment 8856211 [details]
Bug 1256768 - Fix ESLint errors/warnings in devtools/client/webconsole/console-output.js

https://reviewboard.mozilla.org/r/128156/#review130802

::: commit-message-35c7b:1
(Diff revision 1)
> +Bug 1256768 - Fix ESLint errors/warnings in devtools/client/webconsole/console-output.js

add r=nchevobbe at the end
Pushed to reviewboard. Try run seems okay-ish, at least not related to my changes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c06feeb2d26aeae0f1605bb416b2b86c6147887
Attachment #8856211 - Flags: review?(lclark) → review?(nchevobbe)
Comment on attachment 8856211 [details]
Bug 1256768 - Fix ESLint errors/warnings in devtools/client/webconsole/console-output.js

https://reviewboard.mozilla.org/r/128156/#review147434
Attachment #8856211 - Flags: review?(nchevobbe) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e3fcd5eb06d
Fix ESLint errors/warnings in devtools/client/webconsole/console-output.js r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/1e3fcd5eb06d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.