Closed Bug 1385820 Opened 7 years ago Closed 7 years ago

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

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement)

Product:
Developer Infrastructure
Component:
Lint and Formatting
Version:
3 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: standard8, Assigned: dagasatvik10, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(1 file, 1 obsolete file)

Bug 1385820 - Enable the ESLint no-new-wrappers rule across mozilla-central;
59 bytes, text/x-review-board-request
standard8
: review+
markh
: review+
Details 
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
Hi Mark,
I am a complete novice at OSS contributions. I would like to make this my first bug. Can you help me?
Thanks.
(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
(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.
(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.
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 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.
Attachment #8892542 - Flags: feedback?(markh)
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 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 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 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.
(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.
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.
Assignee: nobody → dagasatvik10
Flags: needinfo?(dagasatvik10)
Attachment #8894079 - Attachment is obsolete: true
Attachment #8894079 - Flags: review?(standard8)
Attachment #8894079 - Flags: review?(markh)
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)
Attachment #8892542 - Flags: review?(markh) → review+
(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.
Flags: needinfo?(dagasatvik10)
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.
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).
(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 :)
Jason, can you have a quick look at the test_jsctypes.js changes in this patch and check they look ok please?
Flags: needinfo?(jorendorff)
@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.
(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.
Flags: needinfo?(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.
Flags: needinfo?(standard8)
Flags: needinfo?(jorendorff)
(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 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
Attachment #8892542 - Flags: review?(standard8) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1c7dd91ea84
Enable the ESLint no-new-wrappers rule across mozilla-central; r=markh,standard8
(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.
https://hg.mozilla.org/mozilla-central/rev/f1c7dd91ea84
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Product: Testing → Firefox Build System
Keywords: good-first-bug
Version: Version 3 → 3 Branch
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: