Remove JSHint "ignore" rules for JSX Loop content

RESOLVED FIXED in Firefox 40

Status

P2
minor
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: NiKo, Assigned: cedric.raudin, Mentored)

Tracking

unspecified
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [tech-debt][good first bug][lang=js])

Attachments

(1 attachment)

Right now we have JSX code in this form:

render: function() {
  /* jshint ignore:start */
  …
  /* jshint ignore:end */
}

We shouldn't do that, as:

- JSHint is not part of any build/validation step atm
- it prevents validating JSX and related transpiled code at all, which is error prone

It's up to developers to properly configure their editor/IDE, eg. using jsxhint[0] to get proper linting of JSX files.

Integrating JSXHint validation as part of the build process is probably an higher goal not part of this bug.

[0]: https://www.npmjs.org/package/jsxhint
Whiteboard: [tech-debt]

Updated

4 years ago
backlog: --- → Fx36+

Updated

4 years ago
backlog: Fx36+ → Fx37+

Updated

4 years ago
backlog: Fx37+ → Fx38?

Updated

4 years ago
backlog: Fx38? → tech-debt
Priority: -- → P2
Setting as good first bug. The changes here are to remove the comments in the jsx files here:

http://mxr.mozilla.org/mozilla-central/search?string=jshint+ignore&find=loop&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

(not in sdk.js).

Note that the js files are generated using jsx. See our developer documentation for more info: https://wiki.mozilla.org/Loop/Development
Mentor: standard8
Whiteboard: [tech-debt] → [tech-debt][good first bug][lang=js]
(Assignee)

Comment 2

3 years ago
Hi, I would like to work on this. I am new to open source contributions so it will be fine if someone could guide me through. I have already build Firefox
Hi Cedric, welcome!

As you've already got Firefox built, that's a good start. If you read comment 1 you need to edit the source to remove the lines highlighted in that link, i.e. the ones that are:

 /* jshint ignore:start */

or

 /* jshint ignore:end */

However, don't do this in the /browser/components/loop/content/shared/libs/sdk.js file. That's a library supplied by a third-party, so we don't alter that.

Once you've done the removals, its a good idea to check that Firefox still runs and that the Firefox Hello panel opens, as does the conversation window (click the Hello button on the toolbar and start a conversation.

Btw, which platform are you working on?

Once you've done that, there's details on how to submit a patch here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

If you attach the patch to the bug, and when you do so, set the review flag to "?" and put ":standard8" in the field it displays, that will then come through to me so I can check it.

If you get stuck or need help, feel free to just ask again or email me direct.
Assignee: nobody → cedric.raudin
(Assignee)

Comment 4

3 years ago
Thanks for your informations.

My platform is: Windows 7, 64bits
(Assignee)

Comment 5

3 years ago
Created attachment 8594976 [details] [diff] [review]
bug-1079697-fix.patch
Attachment #8594976 - Flags: review?(standard8)
Comment on attachment 8594976 [details] [diff] [review]
bug-1079697-fix.patch

Thanks Cedric, this is just right. r=Standard8
Attachment #8594976 - Flags: review?(standard8) → review+
(In reply to Pulsebot from comment #7)
> https://hg.mozilla.org/integration/fx-team/rev/c16c67df97cb

As you'll see from that, I just pushed the change for you. It goes to one of our integration trees and it should be merged to the main line (mozilla-central) within the next 24 hours, at which point this bug will be updated and marked as fixed.

Thanks for the patch, and congratulations on your first commit.
Target Milestone: --- → mozilla40
https://hg.mozilla.org/mozilla-central/rev/c16c67df97cb
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.