1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Mark Banner (:standard8)]

We currently have no-new-wrappers enabled for a few directories, but we should expand it to the whole of the repository.

I'm happy to mentor this bug. There's background on our eslint setups here:

https://developer.mozilla.org/docs/ESLint

Also, details of the rule: http://eslint.org/docs/rules/no-new-wrappers

Here's some approximate steps:

- Add a no-new-wrappers (error) line in recommended.js.
- Remove the no-new-wrappers related lines in the existing .eslintrc.js configs (apart from devtools/):

https://dxr.mozilla.org/mozilla-central/search?q=no-new-wrappers&redirect=false

- Run eslint to find the issues to fix:

./mach lint -l eslint

- Fix the failures (note: the ESLint rules page gives hints as to what needs changing)
- Create a commit and push it to mozreview:

http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html

Comment 1

[Satvik Daga]

Hi Mark,
I am a complete novice at OSS contributions. I would like to make this my first bug. Can you help me?
Thanks.

Comment 2

[Mark Banner (:standard8)]

(In reply to Satvik Daga from comment #1)
> Hi Mark,
> I am a complete novice at OSS contributions. I would like to make this my
> first bug. Can you help me?
> Thanks.

Sure, if you haven't already, you'll want to get a checkout of Firefox and maybe a build as well: https://developer.mozilla.org/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Comment 3

[Satvik Daga]

(In reply to Mark Banner (:standard8) from comment #2)
> (In reply to Satvik Daga from comment #1)
> > Hi Mark,
> > I am a complete novice at OSS contributions. I would like to make this my
> > first bug. Can you help me?
> > Thanks.
> 
> Sure, if you haven't already, you'll want to get a checkout of Firefox and
> maybe a build as well:
> https://developer.mozilla.org/docs/Mozilla/Developer_guide/
> Build_Instructions/Simple_Firefox_build

I have build the firefox, it took a while.
Can you tell me how to proceed next?
Thanks.

Comment 4

[Mark Banner (:standard8)]

(In reply to Satvik Daga from comment #3)
> I have build the firefox, it took a while.
> Can you tell me how to proceed next?
> Thanks.

Great! If you follow the steps I listed in comment 0 that should get you most of the way.

Comment 5

[Satvik Daga]

Attachment: Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;

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

Comment 6

[Mark Banner (:standard8)]

Hi, thank you for the patch. Now I've looked at it, there's a few things I'm uncertain about, so I'll ask some others to help me out.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8892542 [details]
Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;

markh: Can you take a look at the services/ changes please? (and maybe some of the toolkit ones!)

services/sync/modules/engines.js
  let error = String("New data: " + [engineData.version, this.version]);
...
  throw error;

This feels like it should be a let error = new Error(...); to better represent what is being thrown.

Some of the other ones I'm not sure about. They look a bit like Strings being abused as objects and as a result, I don't quite know which way we should go with them.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8892542 [details]
Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;

https://reviewboard.mozilla.org/r/163540/#review169504

::: services/sync/modules/engines.js:1012
(Diff revision 1)
>        engines[this.name] = engineData;
>        metaGlobal.payload.engines = engines;
>        metaGlobal.changed = true;
>      } else if (engineData.version > this.version) {
>        // Don't sync this engine if the server has newer data
> -      let error = new String("New data: " + [engineData.version, this.version]);
> +      let error = String("New data: " + [engineData.version, this.version]);

This isn't going to work as expected as we set attributes on the object. We've got bug 1295510 to fix cases where we do that.

::: services/sync/modules/engines/bookmarks.js:360
(Diff revision 1)
>        let parentName = parent.title || "";
>        if (guidMap[parentName] == null)
>          guidMap[parentName] = {};
>  
>        // If the entry already exists, remember that there are explicit dupes.
> -      let entry = new String(guid);
> +      let entry = String(guid);

ditto here.

::: services/sync/modules/resource.js:304
(Diff revision 1)
>        }
>      } catch (ex) {
>        this._log.debug("Caught exception visiting headers in _onComplete", ex);
>      }
>  
> -    let ret     = new String(data);
> +    let ret     = String(data);

and here :(

::: toolkit/components/contentprefs/tests/unit/test_stringGroups.js:18
(Diff revision 1)
>  
>    // These are the different types of aGroup arguments we'll test.
>    var anObject = {"foo": "bar"};                               // a simple object
>    var uri = ContentPrefTest.getURI("http://www.example.com/"); // nsIURI
>    var stringURI = "www.example.com";                           // typeof = "string"
> -  var stringObjectURI = new String("www.example.com");         // typeof = "object"
> +  var stringObjectURI = String("www.example.com");         // typeof = "object"

IIUC, this is now identical to the line above (and the typeof comment is wrong). IOW, it seems this test is really wanting to test an object.

::: toolkit/components/jsdownloads/test/unit/test_Downloads.js:114
(Diff revision 1)
>    let targetPath = getTempFile(TEST_TARGET_FILE_NAME).path;
>    await Downloads.fetch(httpUrl("source.txt"), targetPath);
>    await promiseVerifyContents(targetPath, TEST_DATA_SHORT);
>  
>    targetPath = getTempFile(TEST_TARGET_FILE_NAME).path;
> -  await Downloads.fetch(new String(httpUrl("source.txt")),
> +  await Downloads.fetch(String(httpUrl("source.txt")),

shouldn't we just avoid using String() at all in these cases?

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:272
(Diff revision 1)
>   */
>  function enforceBoolean(aValue) {
>    if (typeof(aValue) !== "number" && typeof(aValue) !== "boolean") {
>      return null;
>    }
> -  return (new Boolean(aValue)).valueOf();
> +  return (Boolean(aValue)).valueOf();

|!!aValue| seems a common pattern used in cases like this.

::: toolkit/modules/PropertyListUtils.jsm:207
(Diff revision 1)
>    wrapInt64: function PLU_wrapInt64(aPrimitive) {
>      if (typeof(aPrimitive) != "string" && typeof(aPrimitive) != "number")
>        throw new Error("aPrimitive should be a string primitive");
>  
> -    let wrapped = new String(aPrimitive);
> +    let wrapped = String(aPrimitive);
>      Object.defineProperty(wrapped, "__INT_64_WRAPPER__", { value: true });

Same issue here as with the sync code IIUC - I don't think wrapped will actually have the property after this change.

Comment 9

[Satvik Daga]

Comment on attachment 8892542 [details]
Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;

https://reviewboard.mozilla.org/r/163540/#review169504

> This isn't going to work as expected as we set attributes on the object. We've got bug 1295510 to fix cases where we do that.

So how should proceed with it?
Should I change it back to string object or use an error object?

> ditto here.

And what here?
In bug 1295510, it has to became an object with guid and hasDupe but what to do here?

> and here :(

Here too.

> IIUC, this is now identical to the line above (and the typeof comment is wrong). IOW, it seems this test is really wanting to test an object.

Yeah, the comment is wrong.
And since the test needs object, do I change it back to string object?
Or is there any other solution?

> shouldn't we just avoid using String() at all in these cases?

I think so too.
So I should remove the String, right?

> |!!aValue| seems a common pattern used in cases like this.

Yes it is.
So I think I need to remove valueOf from here, right?

> Same issue here as with the sync code IIUC - I don't think wrapped will actually have the property after this change.

The function it is part of basically converts string|number to String object.
So what to do here?
Do I need to disable eslint no-wrapper-rule here?

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8892542 [details]
Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;

https://reviewboard.mozilla.org/r/163540/#review170146

Sorry for the terseness of my previous reply - I thought the patch came from standard8 and guessed he would understand what I mean - I would have clarified some of these points had I noticed the patch came from an external contributor (and thanks for the contribution!)

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8892542 [details]
Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;

https://reviewboard.mozilla.org/r/163540/#review169504

> So how should proceed with it?
> Should I change it back to string object or use an error object?

I think all the changes in the services/sync directory should stay as they are and get an //eslint annotation to avoid the eslint error, and ideally another comment referencing bug 1295510 so it's clear why this is being ignored here.

> And what here?
> In bug 1295510, it has to became an object with guid and hasDupe but what to do here?

Just add an // eslint annotation and comment referencing bug 1295510

> Here too.

As above - //eslint and comment referencing bug 1295510

> Yeah, the comment is wrong.
> And since the test needs object, do I change it back to string object?
> Or is there any other solution?

standard8 might have another idea, but I'd be inclined to use // eslint here too, with a comment saying the test really wants to test a string object.

> I think so too.
> So I should remove the String, right?

I think so, yeah.

> Yes it is.
> So I think I need to remove valueOf from here, right?

yep, although standard8 might need to request review for this part from one of the telemetry team.

> The function it is part of basically converts string|number to String object.
> So what to do here?
> Do I need to disable eslint no-wrapper-rule here?

yes, I think disabling the rule is the right thing here too.

Comment 12

[Satvik Daga]

Attachment: Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;

All changes to /sync directory reverted along with comment referencing bug 1295510
Changes reverted where objects are required
Dropped String and Number wrapper where not needed
Removed valueOf from Boolean return in telemetry

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

Comment 13

[Satvik Daga]

(In reply to Mark Hammond [:markh] from comment #11)
> Comment on attachment 8892542 [details]
> Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;
> 
> https://reviewboard.mozilla.org/r/163540/#review169504
> 
> > So how should proceed with it?
> > Should I change it back to string object or use an error object?
> 
> I think all the changes in the services/sync directory should stay as they
> are and get an //eslint annotation to avoid the eslint error, and ideally
> another comment referencing bug 1295510 so it's clear why this is being
> ignored here.
> 
> > And what here?
> > In bug 1295510, it has to became an object with guid and hasDupe but what to do here?
> 
> Just add an // eslint annotation and comment referencing bug 1295510
> 
> > Here too.
> 
> As above - //eslint and comment referencing bug 1295510
> 
> > Yeah, the comment is wrong.
> > And since the test needs object, do I change it back to string object?
> > Or is there any other solution?
> 
> standard8 might have another idea, but I'd be inclined to use // eslint here
> too, with a comment saying the test really wants to test a string object.
> 
> > I think so too.
> > So I should remove the String, right?
> 
> I think so, yeah.
> 
> > Yes it is.
> > So I think I need to remove valueOf from here, right?
> 
> yep, although standard8 might need to request review for this part from one
> of the telemetry team.
> 
> > The function it is part of basically converts string|number to String object.
> > So what to do here?
> > Do I need to disable eslint no-wrapper-rule here?
> 
> yes, I think disabling the rule is the right thing here too.

I have submitted the patch that you requested.
Please tell me how to proceed next.
Thanks.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Can you please "fold" your patches together - currently we have the first version in one patch, then changes made to the first version in a second patch, whereas what we need is just a single patch with all the changes. Assuming you use hg, look for help on the "fold" or "histedit" commands, which should help you squash them together.

Comment 15

[Satvik Daga]

Comment on attachment 8892542 [details]
Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163540/diff/1-2/

Comment 16

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8892542 [details]
Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;

https://reviewboard.mozilla.org/r/163540/#review170650

LGTM, although you should probably clean up the commit message too - we probably only need the first 3 lines of the current message.

::: browser/base/content/browser.js:4198
(Diff revision 2)
>    // Make sure to remove the 'document-shown' observer in case the window
>    // is being closed right after it was opened to avoid leaking.
>    Services.obs.addObserver(newDocumentShown, "document-shown");
>    Services.obs.addObserver(windowClosed, "domwindowclosed");
>  
> -  var charsetArg = new String();
> +  var charsetArg = String();

I think we can probably kill this line and add a "let " at the start of line 4234 (ie, the var is only used once further down, so I can't see why we can't make it local to that block)

Comment 17

[Satvik Daga]

Comment on attachment 8892542 [details]
Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163540/diff/2-3/

Comment 18

[Satvik Daga]

(In reply to Mark Hammond [:markh] from comment #16)
> Comment on attachment 8892542 [details]
> Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;
> 
> https://reviewboard.mozilla.org/r/163540/#review170650
> 
> LGTM, although you should probably clean up the commit message too - we
> probably only need the first 3 lines of the current message.
> 
> ::: browser/base/content/browser.js:4198
> (Diff revision 2)
> >    // Make sure to remove the 'document-shown' observer in case the window
> >    // is being closed right after it was opened to avoid leaking.
> >    Services.obs.addObserver(newDocumentShown, "document-shown");
> >    Services.obs.addObserver(windowClosed, "domwindowclosed");
> >  
> > -  var charsetArg = new String();
> > +  var charsetArg = String();
> 
> I think we can probably kill this line and add a "let " at the start of line
> 4234 (ie, the var is only used once further down, so I can't see why we
> can't make it local to that block)

I have pushed the patch as you had requested Mark.

Comment 19

[Satvik Daga]

Mark [:markh], I don't know what to do next for this bug.
Can you help me please?
And can you also suggest some other bugs that I could work on.
Thanks.

Comment 20

[Mark Banner (:standard8)]

Hi Satvik, thank you for the updates. I'll give it a look over in the next hour or so and we'll take it from there (I've just got back to my desk & something else is in progress atm).

Comment 21

[Satvik Daga]

(In reply to Mark Banner (:standard8) from comment #20)
> Hi Satvik, thank you for the updates. I'll give it a look over in the next
> hour or so and we'll take it from there (I've just got back to my desk &
> something else is in progress atm).

Sure Mark.
Thanks for telling me.
Let me know if there is any issue :)

Comment 22

[Mark Banner (:standard8)]

Jason, can you have a quick look at the test_jsctypes.js changes in this patch and check they look ok please?

Comment 23

[Mark Banner (:standard8)]

@Satvik: The changes are generally looking good. I'd just like to confirm the ctypes changes with Jason.

I have also triggered a try build so that we can confirm the tests pass before we try and land it. The try tree is currently closed at the moment due to issues, but hopefully it'll open up later on - the link will be on the review page once it triggers.

Comment 24

[Satvik Daga]

(In reply to Mark Banner (:standard8) from comment #23)
> @Satvik: The changes are generally looking good. I'd just like to confirm
> the ctypes changes with Jason.
> 
> I have also triggered a try build so that we can confirm the tests pass
> before we try and land it. The try tree is currently closed at the moment
> due to issues, but hopefully it'll open up later on - the link will be on
> the review page once it triggers.

I saw the link on the review page.
I think the failures are because of the changes in test_jsctypes.
Tell me how to proceed further.
Thanks.

Comment 25

[Mark Banner (:standard8)]

Yes I think so to, lets revert the changes in test_jsctypes.js and add a line at the start of the file:

/* eslint-disable no-new-wrappers */

That should be better.

Comment 26

[Satvik Daga]

Comment on attachment 8892542 [details]
Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163540/diff/3-4/

Comment 27

[Satvik Daga]

(In reply to Mark Banner (:standard8) from comment #25)
> Yes I think so to, lets revert the changes in test_jsctypes.js and add a
> line at the start of the file:
> 
> /* eslint-disable no-new-wrappers */
> 
> That should be better.

I have done the above and pushed the changes.

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 8892542 [details]
Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;

https://reviewboard.mozilla.org/r/163540/#review171104

Thank you for the updates. Try seems happy now. r=Standard8

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1c7dd91ea84
Enable the ESLint no-new-wrappers rule across mozilla-central; r=markh,standard8

Comment 30

[Satvik Daga]

(In reply to Mark Banner (:standard8) (afk 11 - 20 Aug) from comment #28)
> Comment on attachment 8892542 [details]
> Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;
> 
> https://reviewboard.mozilla.org/r/163540/#review171104
> 
> Thank you for the updates. Try seems happy now. r=Standard8

Mark, can you vouch for me on Mozillians.
It would be of great help.
Here is the link - https://mozillians.org/en-US/u/dagasatvik10/ 
Thanks.

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f1c7dd91ea84