1283583 - [ESLint] Clean up server/main, server/child

Status: RESOLVED FIXED

(DevTools :: General, defect)

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1283583 - Clean up code style in devtools/server/child.js.

Review commit: https://reviewboard.mozilla.org/r/61598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61598/

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1283583 - Clean up code style in devtools/server/main.js.

Review commit: https://reviewboard.mozilla.org/r/61600/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61600/

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8766883 [details]
Bug 1283583 - Clean up code style in devtools/server/main.js.

https://reviewboard.mozilla.org/r/61600/#review58474

Thanks for the cleanup.
Looks good if try is green.

::: devtools/server/main.js:88
(Diff revision 1)
>  }
>  
>  loader.lazyRequireGetter(this, "events", "sdk/event/core");
>  
> -var {defer, resolve, reject, all} = require("devtools/shared/deprecated-sync-thenables");
> -this.defer = defer;
> +var SyncPromise = require("devtools/shared/deprecated-sync-thenables");
> +this.SyncPromise = SyncPromise;

It looks like sync promises are only used in main.js now. So there is no need to bind it to `this` anymore.

I'm not sure it is useful to use the sync promises in all the callsites which are left... but that's another topic!

::: devtools/server/main.js:235
(Diff revision 1)
> -      throw "DebuggerServer has not been initialized.";
> +      throw new Error("DebuggerServer has not been initialized.");
>      }
>  
>      if (!this.createRootActor) {
> -      throw "Use DebuggerServer.addActors() to add a root actor implementation.";
> +      throw new Error("Use DebuggerServer.addActors() to add a root actor " +
> +                      "implementation.");

Same also here.

::: devtools/server/main.js:296
(Diff revision 1)
>      if (options) {
>        // Lazy loaded actors
>        let {prefix, constructor, type} = options;
>        if (typeof (prefix) !== "string") {
> -        throw new Error("Lazy actor definition for '" + id + "' requires a string 'prefix' option.");
> +        throw new Error(`Lazy actor definition for '${id}' requires a string ` +
> +                        "'prefix' option.");

Is this an automatic rewrite?
This looks weird.

::: devtools/server/main.js:304
(Diff revision 1)
> -        throw new Error("Lazy actor definition for '" + id + "' requires a string 'constructor' option.");
> +        throw new Error(`Lazy actor definition for '${id}' requires a string ` +
> +                        "'constructor' option.");
>        }
>        if (!("global" in type) && !("tab" in type)) {
> -        throw new Error("Lazy actor definition for '" + id + "' requires a dictionary 'type' option whose attributes can be 'global' or 'tab'.");
> +        throw new Error(`Lazy actor definition for '${id}' requires a dictionary ` +
> +                        "'type' option whose attributes can be 'global' or 'tab'.");

Same for all these 4 throw new Error()

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8766882 [details]
Bug 1283583 - Clean up code style in devtools/server/child.js.

https://reviewboard.mozilla.org/r/61598/#review58478

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> ::: devtools/server/main.js:296
> (Diff revision 1)
> >      if (options) {
> >        // Lazy loaded actors
> >        let {prefix, constructor, type} = options;
> >        if (typeof (prefix) !== "string") {
> > -        throw new Error("Lazy actor definition for '" + id + "' requires a string 'prefix' option.");
> > +        throw new Error(`Lazy actor definition for '${id}' requires a string ` +
> > +                        "'prefix' option.");
> 
> Is this an automatic rewrite?
> This looks weird.

Weird because of the mix of template and regular strings?

Comment 7

[Alexandre Poirot [:ochameau]]

Yes, and also I'm used to see multiline strings with templates. But I imagine that is only used for html where spaces are ignored...

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/61600/#review58474

> It looks like sync promises are only used in main.js now. So there is no need to bind it to `this` anymore.
> 
> I'm not sure it is useful to use the sync promises in all the callsites which are left... but that's another topic!

Okay, I'll remove it from this.  I recall from a previous attempt that we do still depend on the sync behavior, so I won't try to remove sync promises here.

> Same also here.

I did this because of an ESLint rule that disallows freestanding template literals (ones without any substiution).  But I agree it looks strange.  I filed bug 1283886 to change the rules, so I'll convert all lines to templates.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c7f4000780a

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/9b0be364ac54
Clean up code style in devtools/server/child.js. r=ochameau
https://hg.mozilla.org/integration/fx-team/rev/1bbc4c035fd8
Clean up code style in devtools/server/main.js. r=ochameau

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b0be364ac54
https://hg.mozilla.org/mozilla-central/rev/1bbc4c035fd8