Closed Bug 1576768 Opened 2 years ago Closed 1 month ago

[eslint] Turn on JavaScript linting and formatting rules for .sjs files

Categories

(Firefox Build System :: Lint and Formatting, task, P3)

task

Tracking

(firefox95 fixed)

RESOLVED FIXED
95 Branch
Tracking Status
firefox95 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: janey, Mentored)

References

Details

(Whiteboard: [lang=js][good-next-bug])

Attachments

(2 files)

*.sjs files are commonly used to write tests in the tree. It would be nice to have JS linting and prettier formatting for them.

Summary: Turn on JavaScript linting and formatting rules for .sjs files → [eslint] Turn on JavaScript linting and formatting rules for .sjs files
Priority: -- → P3

I'm happy to mentor this. Here's a summary of what needs doing:

  • Create a commit that turns off prettier for the "tables" of arrays in the following files:
    • toolkit/components/passwordmgr/test/authenticate.sjs
    • layout/style/test/redundant_font_download.sjs
    • dom/serviceworkers/test/fetch/deliver-gzip.sjs
    • browser/components/urlbar/tests/browser/authenticate.sjs
    • browser/components/extensions/test/browser/authenticate.sjs
  • Add sjs to the list of extensions in tools/lint/eslint.yml
  • Run ./mach eslint --fix (this may take a while).
  • From the output of the above command, there will be various rules that fail. Update the top-level .eslintrc.js to add a section into override to set those rules as warnings for *.sjs files (e.g. similar to this example but with warn rather than off)
  • Run ./mach eslint again to ensure it now passes.
  • Create a commit of everything excluding the .eslintrc.js and tools/lint/eslint.yml changes. When creating that commit add # ignore-this-changeset onto the second or third line of the commit message.
  • Create a commit of just the .eslintrc.js and tools/lint/eslint.yml changes.

Submit those two commits using moz-phab - they should create two revisions that are attached to this bug.

Mentor: standard8
Whiteboard: [lang=js][good-next-bug]

Hey It's Jane here - I am an Outreachy applicant and would love to work on this one please!Hey It's Jane here - I am an Outreachy applicant and would love to work on this one please!

Hi Jane, as this one is a little bit bigger, I'll assign it to you direct. Please do start working on them though if no-one else has commented already.

Assignee: nobody → jenyabrentnall

Hey Mark! Sounds great! A bit of clarification - when we turning off prettier for tables of arrays do we do same named specifically for table or for the whole files? Like:

ChromeUtils.defineModuleGetter(this, "toBinaryTable", "resource:///browser/components/extensions/test/browser/authenticate.sjs");

Haven't reach step 4 yet, probably will have some q there.
(In reply to Mark Banner (:standard8) from comment #3)

When I run mach lint straight up gives me issue -

  55:1  error  'toBinaryTable' is defined but never used.  no-unused-vars (eslint)

Is it even matter?
When I try a whole name one -

ChromeUtils.defineModuleGetter( this, "authenticate", "resource:///browser/components/extensions/test/browser/authenticate.sjs");

lint still gives same one:

  55:1  error  'authenticate' is defined but never used.

thanks!

(In reply to Evgenia Kotovich from comment #4)

Hey Mark! Sounds great! A bit of clarification - when we turning off prettier for tables of arrays do we do same named specifically for table or for the whole files? Like:

Sorry, I wasn't quite clear enough there. The link was pointing to an example for how to turn prettier off then on again - these are the two highlighted lines.

In the case of toolkit/components/passwordmgr/test/authenticate.sjs we have code laid out a bit like a table, and so we want to disable prettier there, and we need to add the two lines to the existing code, so you'd end up with something like:

// base64 decoder
//
// Yoinked from extensions/xml-rpc/src/nsXmlRpcClient.js because btoa()
// doesn't seem to exist. :-(
/* Convert Base64 data to a string */
/* eslint-disable prettier/prettier */
const toBinaryTable = [
    -1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1,
    -1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,-1,
    -1,-1,-1,-1, -1,-1,-1,-1, -1,-1,-1,62, -1,-1,-1,63,
    52,53,54,55, 56,57,58,59, 60,61,-1,-1, -1, 0,-1,-1,
    -1, 0, 1, 2,  3, 4, 5, 6,  7, 8, 9,10, 11,12,13,14,
    15,16,17,18, 19,20,21,22, 23,24,25,-1, -1,-1,-1,-1,
    -1,26,27,28, 29,30,31,32, 33,34,35,36, 37,38,39,40,
    41,42,43,44, 45,46,47,48, 49,50,51,-1, -1,-1,-1,-1
];
/* eslint-enable prettier/prettier */
const base64Pad = '=';

So in each file, you'll need to find the bit of code that looks a bit like a table, and add the comments to surround it.

Attachment #9245925 - Attachment description: Bug 1576768 [eslint] Turn on JavaScript linting and formatting rules for .sjs files. r=Standard8 → Bug 1576768 - Automatically format .sjs files using prettier. r=Standard8

Adding leave-open as we're going to land the automatic changes while we're still working on the second part.

Keywords: leave-open
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c08aa027893
Automatically format .sjs files using prettier. r=Standard8,agi,zombie,extension-reviewers
Depends on: 1736022
Depends on: 1736058

I took a look and it seems to be that the automated changes use ChromeUtils which isn't currently available to sjs files. As far as I know, there's no reason it can't be made available, so I've filed bug 1736058 to add it.

I'm currently running it on try server run with both patches to make sure there's no other issues:

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

Flags: needinfo?(jenyabrentnall)
Attachment #9245925 - Attachment description: Bug 1576768 - Automatically format .sjs files using prettier. r=Standard8 → Bug 1576768 - Automatically format .sjs files using prettier. r=Standard8!

After the try server run in the previous comment, I noticed that some of the automatic rewrites from Components.utils -> ChromeUtils were wrong for the import cases, as they weren't specifying the global to be imported into.

As this was an issue with the automatic rewrite, I took the liberty of fixing those and I'm pushing an update at the moment. I also fixed a few more cases where there were tables that we don't want formatting - I'd missed those when originally writing comment 0.

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

Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ab6bb03dcc1
Automatically format .sjs files using prettier. r=Standard8,agi,zombie,extension-reviewers

Ok, for some reason that didn't want to use ChromeUtils on Android - I've reverted that part of the change for now, and will file a follow-up later.

Flags: needinfo?(standard8)
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2b5e618de42
Automatically format .sjs files using prettier. r=Standard8,agi,zombie,extension-reviewers
Keywords: leave-open
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b60f98f26664
[eslint] Turn on JavaScript linting and formatting rules for .sjs files. r=Standard8

The issues have been fixed, and we're going to re-land.

Flags: needinfo?(jenyabrentnall)
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9de444475899
[eslint] Turn on JavaScript linting and formatting rules for .sjs files. r=Standard8
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch
Blocks: 1738678
Regressions: 1739708
You need to log in before you can comment on or make changes to this bug.