Add eslint support to presubmit checks

[goodov]

This PR:

1. adds upstream web dev style guide presubmit checks using system-wide node with brave/node_modules.
2. adds npm run presubmit -- --fix parameter to automatically pass it to eslint and perform code formatting instead of dry-running to show errors (i.e. runs npm run format for you).
3. add ignores for .storybook/*, build/*, browser/*, ui/webui/resources* to the current eslint config.

Resolves brave/brave-browser#27236

Submitter Checklist:

• I confirm that no security/privacy review is needed, or that I have requested one
• There is a ticket for my issue
• Used Github auto-closing keywords in the PR description above
• Wrote a good PR/commit description
• Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
• Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
• Checked the PR locally:
  • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
  • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
• Ran git rebase master (if needed)

Reviewer Checklist:

• A security review is not needed, or a link to one is included in the PR description
• New files have MPL-2.0 license header
• Adequate test coverage exists to prevent regressions
• Major classes, functions and non-trivial code blocks are well-commented
• Changes in component dependencies are properly reflected in gn
• Code follows the style guide
• Test plan is specified in PR before merging

After-merge Checklist:

• The associated issue milestone is set to the smallest version that the
  changes has landed on
• All relevant documentation has been updated, for instance:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Custom-Headers
  • https://github.com/brave/brave-browser/wiki/Web-Compatibility-Exceptions-in-Brave
  • https://github.com/brave/brave-browser/wiki/QA-Guide
  • https://github.com/brave/brave-browser/wiki/P3A

Test Plan:

[bridiver]

does this mean we have to make sure we include any plugins used by upstream in our node modules? What are they currently missing that we need?

[bridiver]

I guess not https://github.com/brave/brave-core/pull/16289/files#diff-57765a2557bc107c8e5ef5c7c256163c899314e0e7ad9ec7ba31d9f5188d0bafR7

[bridiver]

Actually it seems like we will because that eslint only applies to certain directories. Isn't there some way to specify additional plugin directories? Or can we make these relative paths? https://github.com/brave/brave-core/pull/16289/files#diff-57765a2557bc107c8e5ef5c7c256163c899314e0e7ad9ec7ba31d9f5188d0bafR69

[goodov]

does this mean we have to make sure we include any plugins used by upstream in our node modules? What are they currently missing that we need?

yes ideally we need to have all modules that upstream uses for their eslint runs.

Currently we're missing only eslint-plugin-jsdoc which is not used by the root src/.eslintrc.js config, so I'm pretty sure we can ignore this until we want to use it on our own.

brave/components/.eslintrc.js actually should not have 'root': true, but currently we need this to gradually inherit upstream rules.

this is the list of upstream modules (located in src/third_party/node/package.json):

{
  "name": "webui-node-modules",
  "version": "1.0.0",
  "author": "[email protected]",
  "dependencies": {
    "@types/chai": "4.2.22",
    "@types/d3": "5.16.0",
    "@types/dom-mediacapture-record": "1.0.11",
    "@types/dom-speech-recognition": "0.0.1",
    "@types/filesystem": "0.0.32",
    "@types/filewriter": "0.0.29",
    "@types/google.analytics": "0.0.42",
    "@types/mocha": "9.0.0",
    "@types/offscreencanvas": "2019.6.4",
    "@types/trusted-types": "1.0.6",
    "@types/w3c-css-typed-object-model-level-1": "20180410.0.4",
    "@types/w3c-image-capture": "1.0.4",
    "@types/webrtc": "0.0.31",
    "@typescript-eslint/eslint-plugin": "5.12.0",
    "@typescript-eslint/parser": "5.12.0",
    "babel-eslint": "10.0.2",
    "eslint": "7.11.0",
    "eslint-plugin-jsdoc": "37.5.1",
    "html-minifier": "4.0.0",
    "rollup": "2.58.0",
    "svgo": "2.8.0",
    "terser": "5.3.3",
    "typescript": "4.6.3"
  }
}

we have this (brave/package.json):

    "@typescript-eslint/eslint-plugin": "4.33.0",
...
    "eslint": "7.32.0",
    "eslint-config-standard-with-typescript": "21.0.1",
    "eslint-plugin-import": "2.25.2",
    "eslint-plugin-jest": "25.2.2",
    "eslint-plugin-licenses": "1.0.2",
    "eslint-plugin-node": "11.1.0",
    "eslint-plugin-promise": "5.1.1",

eslint-config-standard-with-typescript automatically includes @typescript-eslint/parser, so we're missing only eslint-plugin-jsdoc.

[goodov]

Actually it seems like we will because that eslint only applies to certain directories. Isn't there some way to specify additional plugin directories? Or can we make these relative paths?

no, there's no way to use multiple directories for plugins. We need to update/add deps during Chromium rebase if eslint will start to fail.

[petemill]

Let's see if I understand what this is doing:

• Moving current eslint config to components/ so that it only controls that directory
• Ignores upstream eslint for ui/webui/components
• Ignores upstream eslint for .storybook

Does that leave anything left to be lint with upstream's config? Maybe scripts in build/?

If not, I would suggest that instead we keep eslintrc in the root because I don't think we ever want different styleguides within the same repo. Then, incrementally in subsequent PRs, modify the existing eslintrc incrementally to:

• include the ui/webui/resources/ and build/ directories
• be based on upstream's eslint (and standard?)

I'm still unclear if upstream's eslint rules can ever be compatible with our eslint config (might be many contradicting rules since ours is based on Standard). So would be good to separate including upstream's config to a separate PR until we figure that out. We'd never want a situation where we have separate competing rules in the same repo for different directories. I think consistency internal to this repo is more important than consistency with Chromium's repo. It can also be argued that consistency with other Brave TS repositories is more important than with Chromium since we largely use different frontend frameworks and filetypes than Chromium.

[goodov]

Let's see if I understand what this is doing:

• Moving current eslint config to components/ so that it only controls that directory
• Ignores upstream eslint for ui/webui/components
• Ignores upstream eslint for .storybook

That is correct, but this is not the final state.

Does that leave anything left to be lint with upstream's config? Maybe scripts in build/?

If not, I would suggest that instead we keep eslintrc in the root because I don't think we ever want different styleguides within the same repo. Then, incrementally in subsequent PRs, modify the existing eslintrc incrementally to:

• include the ui/webui/resources/ and build/ directories
• be based on upstream's eslint (and standard?)

We absolutely shouldn't have different eslint rules in brave-core.

The plan for this PR:

• isolate brave/components (temporary!), so our usual eslint command does the same thing while we're improving other things.
• prepare other directories (not eslinted before) to apply upstream rules and to integrate prettier with the upstream rules in the next PR.

The next PR:

• should wrap prettier to be run via npm run format and npm run presubmit -- --fix
• should reinstate root .eslintrc.js with our license checker rules
• should --fix "small" directories: .storybook, build, ui/webui/resources
• should not touch browser, components
• should let us decide how good/bad upstream rules are and if we want to alter them

The additional PRs after that:

• should --fix browser (because it will be big)
• remove our custom relaxed rules in components, sync it with upstream rules and --fix

I'm still unclear if upstream's eslint rules can ever be compatible with our eslint config (might be many contradicting rules since ours is based on Standard). So would be good to separate including upstream's config to a separate PR until we figure that out. We'd never want a situation where we have separate competing rules in the same repo for different directories. I think consistency internal to this repo is more important than consistency with Chromium's repo. It can also be argued that consistency with other Brave TS repositories is more important than with Chromium since we largely use different frontend frameworks and filetypes than Chromium.

You can look at issues in brave/components after --fix with upstream rules here: https://gist.github.com/goodov/d9505a421c5089c977e9e1586bbeb51d

I think consistency internal to this repo is more important than consistency with Chromium's repo.

IMO brave-core should be seen and operated as the Chromium repo without distinction.

[petemill]

IMO brave-core should be seen and operated as the Chromium repo without distinction.

Whilst this may be true for the C++, the situation is quite different for frontend code:

1. Across projects at Brave, we use a different Javascript style, based on StandardJS. For example, we do not use ; at the end of lines. That might be the biggest issue here. Whether we move away from that as a company is definitely debatable, but that's where we're at right now. There are repos that developers will be cross-working on and having differing styles is a poor experience. What https://gist.github.com/goodov/d9505a421c5089c977e9e1586bbeb51d ignores is all the changes that the --fix made to the files. It's a lot of ; removal and , addition.

2. We use almost exclusively React and a completely different compile process than chromium, and I'm not sure Chromium's rules cover JSX. In the short term we'll also be using some Svelte in both component/ and browser/resources/, That has its own eslint plugin that we'll want to include so that the TS code in .svelte files at any path gets included.

The next PR:

• should reinstate root .eslintrc.js with our license checker rules
• should --fix "small" directories: .storybook, build, ui/webui/resources

This PR as it stands is assuming we'll get rid of the Brave-style StandardJS rules in favor of the Google style guide. That's not decided across the company, and I think this PR needs to focus on integrating the current rules with presubmit, with a view that the next PRs:

• Expand the current brave eslint rules to brave/browser/, brave/ui and build/ (which is of course long overdue).
• Bring in more of Chromium's eslint rules, eventually merging if possible. Either way, that can be a subsequent debate for the frontend developers.

[goodov]

This PR as it stands is assuming we'll get rid of the Brave-style StandardJS rules in favor of the Google style guide. That's not decided across the company, and I think this PR needs to focus on integrating the current rules with presubmit, with a view that the next PRs:

• Expand the current brave eslint rules to brave/browser/, brave/ui and build/ (which is of course long overdue).
• Bring in more of Chromium's eslint rules, eventually merging if possible. Either way, that can be a subsequent debate for the frontend developers.

Okay, let's focus on integrating of presubmit checks in this PR, that's fine.

But I'd like us to do the right thing during formatter integration and configure it in less error prone style from the start. It should solve all "semicolon and comma" issues automatically, so I don't see why we want to ignore this and just create another iteration in future.

[petemill]

I did a compare and can confirm nothing has changed in this file apart from changing from JSON to JS

[fallaciousreasoning]

LGTM too