Closed Bug 1283583 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(2 files)

No description provided.
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()
Attachment #8766883 - Flags: review?(poirot.alex) → review+
Comment on attachment 8766882 [details]
Bug 1283583 - Clean up code style in devtools/server/child.js.

https://reviewboard.mozilla.org/r/61598/#review58478
Attachment #8766882 - Flags: review?(poirot.alex) → review+
(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?
Flags: needinfo?(poirot.alex)
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...
Flags: needinfo?(poirot.alex)
Summary: [eslint] Clean up server/main, server/child → [ESLint] Clean up server/main, server/child
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.
Pushed by jryans@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/9b0be364ac54
https://hg.mozilla.org/mozilla-central/rev/1bbc4c035fd8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.