Skip to content

Commit

Permalink
eslint: Use new hermes-eslint parser, instead of @babel/eslint-parser
Browse files Browse the repository at this point in the history
This is "the recommended parser for use for linting with Flow code"
  ( https://www.npmjs.com/package/hermes-eslint )
and it's the one most recommended in Flow's guide to enabling Flow
enums, which we'd like to do soon:
  https://flow.org/en/docs/enums/enabling-enums/

Since that Flow-enums guide says "Upgrade hermes-parser to at least
version 0.4.8", add a `resolutions` line to guarantee that.

The hermes-eslint code seems to live here on GitHub:
  https://github.com/facebook/hermes/tree/main/tools/hermes-parser/js/hermes-eslint

With this parser, it will be possible (once we enable Flow enums,
coming up) to define a Flow enum without getting an ESLint error
like
  Parsing error: Unexpected token, expected "{"
.

The new parsing also gives better feedback on existing code. We fix
a new warning and a new error in this commit:
- A warning "'V' is defined but never used" on the type param V at
  src/generics.js:175:54. Sure enough, V isn't used; remove it.
- The eslint-disable for no-use-before-define, at
  src/presence/__tests__/heartbeat-test.js:49:5, isn't suppressing
  any error and can be removed. Yep, the use isn't before the
  definition; remove the eslint-disable.

Finally, repeal a few now-redundant ESLint rules, with comments. For
posterity, here's the full error output that I abbreviated in a
comment on ft-flow/define-flow-type:

  Running lint...

  Oops! Something went wrong! :(

  ESLint: 8.17.0

  TypeError: globalScope.__defineGeneric is not a function
  Occurred while linting /Users/chrisbobbe/dev/zulip-mobile/src/api/modelTypes.js:53
  Rule: "ft-flow/define-flow-type"
      at makeDefined (/Users/chrisbobbe/dev/zulip-mobile/node_modules/eslint-plugin-ft-flow/dist/rules/defineFlowType.js:21:21)
      at GenericTypeAnnotation (/Users/chrisbobbe/dev/zulip-mobile/node_modules/eslint-plugin-ft-flow/dist/rules/defineFlowType.js:67:9)
      at ruleErrorHandler (/Users/chrisbobbe/dev/zulip-mobile/node_modules/eslint/lib/linter/linter.js:1114:28)
      at /Users/chrisbobbe/dev/zulip-mobile/node_modules/eslint/lib/linter/safe-emitter.js:45:58
      at Array.forEach (<anonymous>)
      at Object.emit (/Users/chrisbobbe/dev/zulip-mobile/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
      at NodeEventGenerator.applySelector (/Users/chrisbobbe/dev/zulip-mobile/node_modules/eslint/lib/linter/node-event-generator.js:297:26)
      at NodeEventGenerator.applySelectors (/Users/chrisbobbe/dev/zulip-mobile/node_modules/eslint/lib/linter/node-event-generator.js:326:22)
      at NodeEventGenerator.enterNode (/Users/chrisbobbe/dev/zulip-mobile/node_modules/eslint/lib/linter/node-event-generator.js:340:14)
      at CodePathAnalyzer.enterNode (/Users/chrisbobbe/dev/zulip-mobile/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:795:23)
  • Loading branch information
chrisbobbe authored and gnprice committed Aug 3, 2022
1 parent 16d1182 commit 3b76083
Show file tree
Hide file tree
Showing 7 changed files with 52 additions and 11 deletions.
30 changes: 27 additions & 3 deletions .eslintrc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
# $ git diff --no-index /tmp/{foo,bar}.json


parser: "@babel/eslint-parser"
parser: "hermes-eslint"

extends:
- airbnb
Expand Down Expand Up @@ -75,6 +75,14 @@ rules:
# of Airbnb's settings where we don't want a full repeal.
#

# With hermes-eslint for `parser`, above, this rule can analyze Flow
# types. It would fire on global types like $ReadOnly, for which it
# doesn't see a declaration. We could find out how to teach the rule that
# these types are globally defined…or just turn the rule off, because it's
# redundant with Flow (specifically `Flow(cannot-resolve-name)`, I think):
# https://github.com/flow-typed/eslint-plugin-ft-flow/issues/33#issue-1274050123
no-undef: off # https://eslint.org/docs/latest/rules/no-undef

# Tricky-looking code. These are all sometimes perfectly legitimate.
no-bitwise: off
no-confusing-arrow: off
Expand Down Expand Up @@ -284,6 +292,13 @@ rules:
# React; plugin `react`.
#

# These two are workarounds to suppress no-unused-vars when the parser
# doesn't understand JSX. Our parser (hermes-eslint, set above) does, so
# they're unnecessary:
# https://github.com/flow-typed/eslint-plugin-ft-flow/issues/33#issue-1274050123
react/jsx-uses-react: off # https://github.com/jsx-eslint/eslint-plugin-react/blob/v7.30.0/docs/rules/jsx-uses-react.md
react/jsx-uses-vars: off # https://github.com/jsx-eslint/eslint-plugin-react/blob/v7.30.0/docs/rules/jsx-uses-vars.md

react/jsx-filename-extension: off # Like RN upstream, we call the files *.js.

react/destructuring-assignment: off # The opposite is often much cleaner.
Expand Down Expand Up @@ -324,8 +339,18 @@ rules:
# Flow; plugin `ft-flow`.
#


# These two are workarounds to suppress no-undef and no-unused-vars,
# respectively, when the parser doesn't understand Flow types. Our parser
# (hermes-eslint, set above) does, so they're unnecessary:
# https://github.com/flow-typed/eslint-plugin-ft-flow/issues/33#issue-1274050123
# In fact, linting breaks if I leave define-flow-type on with this parser:
# TypeError: globalScope.__defineGeneric is not a function
# (See commit message for full error output.)
ft-flow/define-flow-type: off # https://github.com/flow-typed/eslint-plugin-ft-flow/tree/v2.0.1#define-flow-type
ft-flow/use-flow-type: off # https://github.com/flow-typed/eslint-plugin-ft-flow/tree/v2.0.1#use-flow-type

ft-flow/boolean-style: [error, boolean]
ft-flow/define-flow-type: warn
ft-flow/delimiter-dangle: off
ft-flow/no-dupe-keys: error
ft-flow/no-primitive-constructor-types: error
Expand All @@ -341,7 +366,6 @@ rules:
# For more formatting rules, see tools/formatting.eslintrc.yaml.
ft-flow/type-id-match: [error, '^([A-Z][a-z0-9]+)+$']
ft-flow/union-intersection-spacing: [error, always]
ft-flow/use-flow-type: warn
ft-flow/valid-syntax: warn

overrides:
Expand Down
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@
"flow-bin": "^0.162.0",
"flow-coverage-report": "^0.8.0",
"flow-typed": "^3.3.1",
"hermes-eslint": "^0.9.0",
"immutable-devtools": "^0.1.5",
"jest": "^26.6.3",
"jest-cli": "^26.4.1",
Expand All @@ -127,6 +128,7 @@
"yarn-deduplicate": "^3.0.0"
},
"resolutions": {
"hermes-parser": ">=0.4.8",
"jest-expo/react-test-renderer": "17.0.2",
"prettier-eslint-cli/prettier-eslint": "^15.0.0",
"react-native/use-subscription": ">=1.0.0 <1.6.0"
Expand Down
2 changes: 1 addition & 1 deletion src/generics.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ export type ReadWrite<T: $ReadOnly<{ ... }>> = $Diff<T, {||}>;
// $Diff and other "type destructors", which as of 2020-08 the Flow team is
// taking up as a priority:
// https://github.com/facebook/flow/issues/7886#issuecomment-669977952
export type SubsetProperties<T, U> = $ObjMapi<U, <K, V>(K) => T[K]>;
export type SubsetProperties<T, U> = $ObjMapi<U, <K>(K) => T[K]>;

/**
* Assert a contradiction, statically. Do nothing at runtime.
Expand Down
1 change: 0 additions & 1 deletion src/presence/__tests__/heartbeat-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ describe('Heartbeat', () => {
return this.heartbeat.isActive();
}

// eslint-disable-next-line no-use-before-define
static getExtant(): $ReadOnlyArray<JestHeartbeatHelper> {
return this._currentHeartbeats;
}
Expand Down
2 changes: 1 addition & 1 deletion tools/flow-todo.eslintrc.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
parser: "@babel/eslint-parser"
parser: hermes-eslint
plugins:
- ft-flow
rules:
Expand Down
2 changes: 1 addition & 1 deletion tools/formatting.eslintrc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# we can follow each Prettier run with a streamlined (and faster) ESLint run
# that uses only these rules.

parser: "@babel/eslint-parser"
parser: "hermes-eslint"

plugins:
- ft-flow
Expand Down
24 changes: 20 additions & 4 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -6006,10 +6006,26 @@ hermes-engine@~0.9.0:
resolved "https://registry.yarnpkg.com/hermes-engine/-/hermes-engine-0.9.0.tgz#84d9cfe84e8f6b1b2020d6e71b350cec84ed982f"
integrity sha512-r7U+Y4P2Qg/igFVZN+DpT7JFfXUn1MM4dFne8aW+cCrF6RRymof+VqrUHs1kl07j8h8V2CNesU19RKgWbr3qPw==

[email protected]:
version "0.4.7"
resolved "https://registry.yarnpkg.com/hermes-parser/-/hermes-parser-0.4.7.tgz#410f5129d57183784d205a0538e6fbdcf614c9ea"
integrity sha512-jc+zCtXbtwTiXoMAoXOHepxAaGVFIp89wwE9qcdwnMd/uGVEtPoY8FaFSsx0ThPvyKirdR2EsIIDVrpbSXz1Ag==
hermes-eslint@^0.9.0:
version "0.9.0"
resolved "https://registry.yarnpkg.com/hermes-eslint/-/hermes-eslint-0.9.0.tgz#f1423abe3dfd959257430d61a9bccd4700b59e09"
integrity sha512-rlkK51UpGwo0ZWg8hu8DVICth7RfGSvaEJzFflos8bDOYm/d842/J3IXi0lB9R9waOp4VGGSc8VDmh+a9p2Q2w==
dependencies:
esrecurse "^4.3.0"
hermes-estree "0.9.0"
hermes-parser "0.9.0"

[email protected]:
version "0.9.0"
resolved "https://registry.yarnpkg.com/hermes-estree/-/hermes-estree-0.9.0.tgz#026e0abe6db1dcf50a81a79014b779a83db3b814"
integrity sha512-5DZ7Y0CbHVk8zPqgRCvqp8iw+P05svnQDI1aJFjdqCfXJ/1CZ+8aYpGlhJ29zCG5SE5duGTzSxogAYYI4QqXqw==

[email protected], [email protected], hermes-parser@>=0.4.8:
version "0.9.0"
resolved "https://registry.yarnpkg.com/hermes-parser/-/hermes-parser-0.9.0.tgz#ede3044d50479c61843cef5bbdcea83933d4e4ec"
integrity sha512-IcvJIlAn+9tpHkP+HTsxWKrIdQPp0gvGrrQmxlL4XnNS+Oh6R/Fpxbcoflm2kY3zgQjEvxZxLiK/2+k3/5wsrw==
dependencies:
hermes-estree "0.9.0"

hermes-profile-transformer@^0.0.6:
version "0.0.6"
Expand Down

0 comments on commit 3b76083

Please sign in to comment.