js: Update prettier-eslint, etc., to latest!! #5393

chrisbobbe · 2022-05-26T02:21:05Z

This is wip because we should find out what we actually want from these new versions. :) Posting this as a draft so we can discuss.

gnprice

Thanks @chrisbobbe for taking care of this!

The first N-3 commits all look good to me:
ace1ab2 js [nfc]: Remove some useless param defaulting
c9665fa react: Consistently use named function declarations for React components
af12e74 react [nfc]: Remove a useless Fragment
2cf1f82 react [nfc]: Stop using arrow functions for lifecycle methods
589429d js [nfc]: Consistently use export default, not module.exports =
2ea03fc js [nfc]: Align code with preference for using regex literals
2a830d7 react [nfc]: Remove unused React context invocations

modulo small comments below.

Looking at the WIP last few now:
86058d8 default-param-last ignores for reducers
3db2f86 wip js: Update prettier-eslint, etc., to latest!!
b79128d eslint: Enforce no unused eslint-disable comments

gnprice · 2022-05-27T02:14:11Z

src/streams/StreamIcon.js

+export default function StreamIcon({
+  color,
+  style,
+  isPrivate,
+  isMuted,
+  isWebPublic,
+  size,
+}: Props): Node {


nit: perhaps a good case for the const { color, style, isPrivate, isMuted, isWebPublic, size } = props; form instead, particularly if that gets it back to one line

gnprice · 2022-05-27T02:25:05Z

src/common/Icons.js

@@ -105,4 +106,5 @@ export const IconAttach: SpecificIconType = makeIcon(Feather, 'paperclip');
 export const IconAttachment: SpecificIconType = makeIcon(IoniconsIcon, 'document-attach-outline');
 export const IconGroup: SpecificIconType = makeIcon(FontAwesome, 'group');
 export const IconPlus: SpecificIconType = makeIcon(Feather, 'plus');
+// eslint-disable-next-line react/function-component-definition


nit at branch level:

I caught these while experimenting with a new(ish eslint rule: react/function-component-definition https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/function-component-definition.md

I think my ideal format for making this kind of lint-rule-directed fix is to have the same commit that fixes the things it finds also enable the rule. So e.g. the branch would first add or upgrade the plugin that makes the rule available; in the commit that does so, disable any new rules (that affect anything) in order to avoid touching code; then later commits enable the rules one at a time.

For an example of that format, you can look at the series in TsFlower where I added ESLint: the last N commits at the top of https://github.com/gnprice/tsflower/commits/2a815a21523dca1b498d47397a9cca9f0cc6bc68, in particular gnprice/tsflower@ac22836 which enabled it but with several rules disabled, then various commits like gnprice/tsflower@df966a2 removing a disable and fixing some code, or gnprice/tsflower@2a815a2 leaving a rule disabled but changing the comment from saying "plan to investigate and enable" to saying "disabled because such-and-such".

A bonus is that eslint-disable lines like this appear only at the same time as they become needed (while also still appearing in the same commit that handles the other instances of the same lint rule.)

But these commits are already quite clear, so if it'd be a pain to revise the branch into that form then this is fine.

gnprice

OK, and read the rest! Small comments below.

The formatting changes from the Prettier upgrade look fine. Some code gets nicer (like foo.bar().baz() going on one line), some gets less nice (like a lot of stuff getting an extra level of indentation). I'm not sure I like it better on net; but Prettier is designed as a take-it-or-leave-it package, and I don't have a better alternative nor want to be stuck on an old version. So 🤷

There's those three new lint rules that get disabled. I'll take a look at what those are.

gnprice · 2022-05-27T02:33:58Z

src/streams/StreamIcon.js

@@ -1,4 +1,5 @@
 /* @flow strict-local */
+/* eslint-disable no-shadow */


This seems unintended for the upgrade commit.

gnprice · 2022-05-27T02:35:10Z

tools/fmt

-    --eslint-config-path ./tools/formatting.eslintrc.yaml \
+    --eslint-config-path $root_dir/tools/formatting.eslintrc.yaml \


This is a good fix that seems like it can be in a separate prep commit. (Ditto in tools/test below.)

Oh, but I missed a bug in this fix: the $-interpolation should always be in double quotes. Otherwise its value gets interpreted by the shell in various exciting ways. Discussion here:
https://www.shellcheck.net/wiki/SC2086

("Always" unless those exciting interpretations are intended, that is. But that's rare.)

(We should probably start systematically using ShellCheck.)

Also a nit: I've moved toward a style of always using {} around variable interpolation, even when it's not required. Helps to highlight that this is a variable reference, not just a literal string like most of a command line usually is -- and also helps avoid bugs when the braces are needed for terminating the variable name. E.g., sep=":"; echo "1$sep2" prints "1" (or errors out, if you have set -u), not "1:2".

Mm, thanks for catching these!

gnprice · 2022-05-27T02:36:22Z

.eslintrc.yaml

+  no-promise-executor-return: off # new Promise(r => setTimeout(r), 1000);
+
+  # Reasonable, but a bit noisy.
+  class-methods-use-this: off


(I'll go read about these rules, and the other mentioned below.)

gnprice · 2022-05-27T02:40:14Z

src/compose/ComposeBox.js

        // https://github.com/eslint/eslint/issues/11045
-        // eslint-disable-next-line no-unused-expressions
        mentionWarnings.current?.handleMentionSubscribedCheck(completion);


This comment seems probably meant to explain the eslint-disable.

gnprice

OK, and done reading about those rules.

Hmm, other things I notice in VS Code, though:

I don't seem to get ESLint feedback in the IDE.
When I auto-format, it reverts the formatting changes.

Is it all working there for you? Perhaps I need to restart the IDE and/or upgrade the plugin.

gnprice · 2022-05-27T02:50:27Z

.eslintrc.yaml

@@ -204,6 +204,10 @@ rules:
  prefer-destructuring:
    - error
    - AssignmentExpression: {array: false, object: false}
+  no-promise-executor-return: off # new Promise(r => setTimeout(r), 1000);


(ITYM new Promise(r => setTimeout(r, 1000)))

https://eslint.org/docs/rules/no-promise-executor-return

Hmm, it would be nice to catch possible mistakes like this example from the docs:

new Promise((resolve, reject) => { if (someCondition) { return defaultResult; } getSomething((err, result) => { if (err) { reject(err); } else { resolve(result); } }); });

For this setTimeout example where the rule is annoying, one answer would be to be explicit about discarding the return value, with the void operator:
new Promise(r => void setTimeout(r, 1000))

How many places does this rule complain about?

one answer would be to be explicit about discarding the return value, with the void operator:
new Promise(r => void setTimeout(r, 1000))

Ha, that doesn't even work -- the rule still complains.

Yeah, let's skip this rule.

gnprice · 2022-05-27T02:58:23Z

.eslintrc.yaml

+  # Reasonable, but a bit noisy.
+  class-methods-use-this: off


Yeah, this seems more noisy than helpful.

Basically it's trying to say you should make a method be a static method whenever its implementation would work as a static method. But that isn't good advice: whether the method is static or not is an API choice.

The large number of exceptions that the config we inherit (from eslint-config-airbnb? yep, looks like it) has to write down:

$ npx eslint --print-config src/types.js | jq '.rules["class-methods-use-this"]' [ "error", { "exceptMethods": [ "render", "getInitialState", "getDefaultProps", "getChildContext", "componentWillMount", "UNSAFE_componentWillMount", "componentDidMount", "componentWillReceiveProps", "UNSAFE_componentWillReceiveProps", "shouldComponentUpdate", "componentWillUpdate", "UNSAFE_componentWillUpdate", "componentDidUpdate", "componentWillUnmount", "componentDidCatch", "getSnapshotBeforeUpdate" ], "enforceForClassFields": true } ]

is a pretty clear signal that this is not an especially principled rule.

gnprice · 2022-05-27T03:07:08Z

.eslintrc.yaml

  react/prefer-stateless-function: off
  react/sort-comp: off
+  react/no-unstable-nested-components: off


https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-unstable-nested-components.md

Hmm, yeah, the situation this looks for does sound bad.

Running it…

✖ 19 problems (19 errors, 0 warnings)

(in 7 different files)

Yeah, probably a followup commit (or a few commits) to fix these would be good.

I think most of them can just insert a useMemo and be done. In Title.js it's a bit more than that because they're conditional… hmm, no they're not, I think the rule is just getting a false positive. So an eslint-disable for the file should do it.

Cool, yeah; I'll aim to come back to this.

Following the eagerly awaited resolution of prettier/prettier-eslint-cli#304 . And adjust our ESLint config so that we can do this with no new ESLint errors or suppressions. After this, we'll un-ignore the rules under "New rules we want", one by one, which will mean fixing the code that doesn't yet follow each rule. Greg says, about the formatting changes: zulip#5393 (review) > The formatting changes from the Prettier upgrade look fine. Some > code gets nicer (like `foo.bar().baz()` going on one line), some > gets less nice (like a lot of stuff getting an extra level of > indentation). I'm not sure I like it better on net; but Prettier > is designed as a take-it-or-leave-it package, and I don't have a > better alternative nor want to be stuck on an old version. So > 🤷 The added @babel/* deps are to satisfy peer-dependency requirements. The resolutions line gets us prettier/prettier-eslint#749 and can be removed when prettier-eslint-cli bumps its prettier-eslint version.

chrisbobbe · 2022-06-03T19:04:53Z

Thanks for the review! Revision pushed.

js: Fix errors in tools/fmt --all by adding plugins to formatting config

Just before and at this first commit, the effective ESLint config from tools/formatting.eslintrc.yaml—i.e., the output of yarn eslint --print-config tools/formatting.eslintrc.yaml—only differs in the order of the items in the array at .plugins:

Before:

    "plugins": [
        "import",
        "react",
        "jsx-a11y",
        "react-hooks",
        "flowtype",
        "jest"
    ]

At commit:

    "plugins": [
        "jsx-a11y",
        "react-hooks",
        "react",
        "import",
        "flowtype",
        "jest"
    ]

I guess that's a good sign. The concern to have about adding stuff to this config file is that it loads a bunch of additional things to check, making tools/fmt slow.

I'd be interested to know if the errors I quoted in that commit reproduce for you, though. Possibly there's some state that I've failed to reset, and we shouldn't actually treat these as errors at main.

I also don't yet know why lint and prettier are failing in CI; they aren't failing locally for me. :(

chrisbobbe · 2022-06-03T19:10:13Z

I don't seem to get ESLint feedback in the IDE.

When I auto-format, it reverts the formatting changes.

Is it all working there for you? Perhaps I need to restart the IDE and/or upgrade the plugin.

Yeah, try restarting the IDE. After doing that just now, I don't seem to have either of these problems. I don't expect to ever be able to upgrade past version 4.7.0 of esbenp.prettier-vscode; see discussion.

gnprice

Thanks for the revision! All generally looks good; a few comments below.

Also, do you still intend this to be marked as a draft PR?

gnprice · 2022-06-07T01:54:52Z

tools/fmt

-    --eslint-config-path ./tools/formatting.eslintrc.yaml \
+    --eslint-config-path $root_dir/tools/formatting.eslintrc.yaml \


Oh, but I missed a bug in this fix: the $-interpolation should always be in double quotes. Otherwise its value gets interpreted by the shell in various exciting ways. Discussion here:
https://www.shellcheck.net/wiki/SC2086

("Always" unless those exciting interpretations are intended, that is. But that's rare.)

(We should probably start systematically using ShellCheck.)

gnprice · 2022-06-07T01:59:31Z

tools/fmt

-    --eslint-config-path ./tools/formatting.eslintrc.yaml \
+    --eslint-config-path $root_dir/tools/formatting.eslintrc.yaml \


Also a nit: I've moved toward a style of always using {} around variable interpolation, even when it's not required. Helps to highlight that this is a variable reference, not just a literal string like most of a command line usually is -- and also helps avoid bugs when the braces are needed for terminating the variable name. E.g., sep=":"; echo "1$sep2" prints "1" (or errors out, if you have set -u), not "1:2".

gnprice · 2022-06-07T02:04:54Z

tools/test

@@ -185,7 +185,7 @@ run_lint() {
    local files
    files=( $(apply_eslintignore "$@") )
    (( ${#files[@]} )) || return 0
-    eslint ${fix:+--fix} --max-warnings=0 "${files[@]}"
+    eslint ${fix:+--fix} --report-unused-disable-directives --max-warnings=0 "${files[@]}"


Hmm interesting; what's the reasoning for putting this here instead of in the eslintrc?

I think one difference in resulting behavior will be that the reports won't appear in the IDE, only when running tools/test.

I almost wrote down the reason, but it was getting complicated…if you want, you can try it, and be sure to run both tools/test prettier and tools/fmt. I don't think I found a way to put it in the config such that it worked.

ISTR if we put it in the config, prettier-eslint ends up passing it to an eslint API (a constructor function?) in the wrong form. When I got error output suggesting that, I tried writing down the expected form in the config file. But then the config file didn't pass validation.

Ha, I see. If I write reportUnusedDisableDirectives: true, then ESLint is happy (and I get warnings as expected in the IDE), but:

$ tools/test prettier --diff @ Running prettier... prettier-eslint [ERROR]: There was trouble creating the ESLint CLIEngine. prettier-eslint-cli [ERROR]: There was an error formatting "/home/greg/z/mobile/src/events/doEventActionSideEffects.js": Error: Invalid Options: - 'reportUnusedDisableDirectives' must be any of "error", "warn", "off", and null. …

If I follow that error message and write, say, reportUnusedDisableDirectives: error, then tools/test prettier is happy, but:

$ tools/test lint --diff @ Running lint... Oops! Something went wrong! :( ESLint: 8.16.0 Error: ESLint configuration in .eslintrc.yaml is invalid: - Property "reportUnusedDisableDirectives" is the wrong type (expected boolean but got `"error"`). …

Odd because looking at the stack traces, it seems like it's two different APIs of ESLint that are requiring incompatible ways of writing that option:

prettier-eslint-cli [ERROR]: There was an error formatting "/home/greg/z/mobile/src/events/doEventActionSideEffects.js": Error: Invalid Options: - 'reportUnusedDisableDirectives' must be any of "error", "warn", "off", and null. at processOptions (/home/greg/z/mobile/node_modules/eslint/lib/eslint/eslint.js:281:15) at new ESLint (/home/greg/z/mobile/node_modules/eslint/lib/eslint/eslint.js:428:34) at getESLint (/home/greg/z/mobile/node_modules/prettier-eslint/dist/utils.js:386:12) …

vs.

ESLint: 8.16.0 Error: ESLint configuration in .eslintrc.yaml is invalid: - Property "reportUnusedDisableDirectives" is the wrong type (expected boolean but got `"error"`). at ConfigValidator.validateConfigSchema (/home/greg/z/mobile/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2156:19) at ConfigArrayFactory._normalizeConfigData (/home/greg/z/mobile/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2982:19) at ConfigArrayFactory.loadInDirectory (/home/greg/z/mobile/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2850:33) … at CascadingConfigArrayFactory.getConfigArrayForFile (/home/greg/z/mobile/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3754:18) at FileEnumerator._iterateFilesWithFile (/home/greg/z/mobile/node_modules/eslint/lib/cli-engine/file-enumerator.js:366:43)

🤷 Not worth spending the time to debug, I think -- we'll just tolerate not getting this particular warning in the IDE.

gnprice · 2022-06-07T02:19:00Z

src/common/Icons.js

 const makeIcon = <Glyphs: string>(
  iconSet: ComponentType<IconPropsBusted<Glyphs>>,
  name: Glyphs,
-): SpecificIconType => props => {
-  const FixedIcon = fixIconType<Glyphs>(iconSet);
-  return <FixedIcon name={name} {...props} />;
-};
+): SpecificIconType =>
+  function RNVIIcon(props) {


This definition seems to thrash a bit: it gets reformatted more in the main upgrade commit, then reformatted partially back in the "Enable react/function-component-definition" commit.

I guess the latter commit changes props => { to function RNVIIcon(props) {. Maybe that causes the change? Seems like a bit of an annoying Prettier instability if so.

(but, to be clear, one of many -- not a regression to worry about)

gnprice · 2022-06-07T02:20:08Z

src/ZulipMobile.js

-  // If using react-native-gesture-handler directly, not just via React
-  // Navigation, we should use a GestureHandlerRootView; see
-  //  https://docs.swmansion.com/react-native-gesture-handler/docs/1.10.3/#js
-  //
-  // React Nav seems to have followed the following advice, in that doc:
-  //   > If you're using gesture handler in your component library, you may
-  //   > want to wrap your library's code in the GestureHandlerRootView
-  //   > component. This will avoid extra configuration for the user.
-  // which I think is why they don't mention GestureHandlerRootView in their
-  // own setup doc, and why I think it's probably fine to omit it if we're
-  // not using r-n-gesture-handler directly ourselves.


This comment gets deleted. Was that intended?

Odd. No, not intended. Thanks for catching it!

gnprice · 2022-06-07T02:38:45Z

tools/formatting.eslintrc.yaml

 plugins:
  - flowtype
+  - import
+  - jest
+  - react


Cool. The one downside of a change like this is that it makes a formatting run slower, by giving it more code to load. But I just tested, and the difference is tolerable:

Before:

$ echo >>src/ZulipMobile.js ; time tools/fmt success formatting 1 file with prettier-eslint time: 3.988s wall (4.999s u, 0.240s s) $ echo >>src/ZulipMobile.js ; time tools/fmt success formatting 1 file with prettier-eslint time: 4.022s wall (5.130s u, 0.220s s)

After:

$ echo >>src/ZulipMobile.js ; time tools/fmt success formatting 1 file with prettier-eslint time: 4.275s wall (5.452s u, 0.288s s) $ echo >>src/ZulipMobile.js ; time tools/fmt success formatting 1 file with prettier-eslint time: 4.202s wall (5.303s u, 0.229s s)

So like 200-300ms longer, which is enough time to be noticeable… except that it's much more annoyingly slow already at 4.0s.

So increasing that by 5-7%, to 4.2s or 4.3s, is worth it to get rid of those spurious "Definition for rule '…' was not found" errors.

The one downside of a change like this is that it makes a formatting run slower, by giving it more code to load. But I just tested, and the difference is tolerable […]

Thanks for running the timer! If these three lines are causing more code to be loaded, I guess I'm not sure how the resulting config ends up being so similar with and without the lines: #5393 (comment)

Thanks for running the timer! If these three lines are causing more code to be loaded, I guess I'm not sure how the resulting config ends up being so similar with and without the lines: #5393 (comment)

Ah. Here's that command:

yarn eslint --print-config tools/formatting.eslintrc.yaml

But that's not doing what you want it to be doing. That's saying: what is the ESLint config for linting the file tools/formatting.eslintrc.yaml. IOW, --print-config doesn't take an argument.

Here's notes from the top of our eslintrc:

# Tips for developing on this file: # # * To see what config currently applies, try a command like: # # $ npx eslint --print-config src/types.js | less # # or for a specific rule: # # $ npx eslint --print-config src/types.js | jq '.rules["no-continue"]'

And here's the eslint command line from tools/fmt:

eslint --no-eslintrc -c tools/formatting.eslintrc.yaml --fix "${files[@]}"

So, with our regular ESLint config:

$ node_modules/.bin/eslint --print-config src/types.js | jq .plugins [ "jsx-a11y", "react-hooks", "react", "import", "flowtype", "jest" ]

With the formatting-only config, at the tip of this branch:

$ node_modules/.bin/eslint --no-eslintrc -c tools/formatting.eslintrc.yaml --print-config src/types.js | jq .plugins [ "react", "jest", "import", "flowtype" ]

With the formatting-only config, before this branch:

$ node_modules/.bin/eslint --no-eslintrc -c tools/formatting.eslintrc.yaml --print-config src/types.js | jq .plugins [ "flowtype" ]

gnprice · 2022-06-07T02:43:00Z

Yeah, try restarting the IDE.

(And indeed, that fixed both those problems.)

chrisbobbe · 2022-06-07T02:57:40Z

Also, do you still intend this to be marked as a draft PR?

I left it as a draft when I noticed that CI was failing, even though the tests don't fail for me locally. :(

gnprice · 2022-06-07T20:58:59Z

I left it as a draft when I noticed that CI was failing, even though the tests don't fail for me locally. :(

Hmm, I see:

Running lint...

/home/runner/work/zulip-mobile/zulip-mobile/src/api/permissionsTypes.js
  65:1  error  Unused eslint-disable directive (no problems were reported from 'flowtype/type-id-match')

✖ 1 problem (1 error, 0 warnings)
…
Running prettier...
/home/runner/work/zulip-mobile/zulip-mobile/src/__tests__/permissionSelectors-test.js
/home/runner/work/zulip-mobile/zulip-mobile/src/api/permissionsTypes.js
/home/runner/work/zulip-mobile/zulip-mobile/src/realm/__tests__/realmReducer-test.js
success formatting 3 files with prettier-eslint
527 files were unchanged

Yeah, I guess that will call for some debugging.

(And I don't reproduce either. For me:

$ tools/test --all lint prettier
Running lint...
Running prettier...
530 files were unchanged
Passed!

)

gnprice · 2022-06-07T21:22:46Z

OK, I think I've debugged the issue. It's basically a merge conflict -- one that was action-at-a-distance and so Git didn't catch.

The first clue is that looking at that first error message:

/home/runner/work/zulip-mobile/zulip-mobile/src/api/permissionsTypes.js
  65:1  error  Unused eslint-disable directive (no problems were reported from 'flowtype/type-id-match')

what's line 65 of that file at the tip of the branch? Here's it and its immediate neighbors:

  Nobody: (6: 6),
  OwnerOnly: (7: 7),
};

Hmmm. So it seems like it's operating on some different version of that file somehow.

Then I poked around the "checkout" step at the top. And:

Checking out the ref
  /usr/bin/git checkout --progress --force refs/remotes/pull/5393/merge

  …

  HEAD is now at 341d3d3 Merge 883e1b0254fbad82137b9ea2a901bf2779a5ba57 into e398e28143acb5496eb4dc6f6776a6c16730a1a6
/usr/bin/git log -1 --format='%H'
'341d3d3a2e1f2502b0d35bcaf6ebdb944fb0e90f'

Aha. It's that it's running on the merge of this PR branch and the upstream HEAD.

The pull/5393/merge branch has moved on since then as we've merged more PRs. But here's a repro:

$ git co e398e2814
$ git merge 883e1b0
Auto-merging src/realm/realmReducer.js
Auto-merging src/realm/__tests__/realmReducer-test.js
Auto-merging src/permissionSelectors.js
Auto-merging src/api/permissionsTypes.js
Merge made by the 'recursive' strategy.

$ time tools/test --all lint prettier
Running lint...

/home/greg/z/mobile/src/api/permissionsTypes.js
  65:1  error  Unused eslint-disable directive (no problems were reported from 'flowtype/type-id-match')

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

Running prettier...
/home/greg/z/mobile/src/__tests__/permissionSelectors-test.js
/home/greg/z/mobile/src/api/permissionsTypes.js
/home/greg/z/mobile/src/realm/__tests__/realmReducer-test.js
success formatting 3 files with prettier-eslint
527 files were unchanged

FAILED: lint prettier

time: 60.209s wall (76.161s u, 1.754s s)

So the solution is just for you to rebase the branch and fix the new errors.

gnprice · 2022-06-07T21:27:45Z

This is a PR that will naturally accumulate conflicts rapidly -- both that Git sees, and that it doesn't but that cause CI failures, as we found just above. It's quite close now, so I think it'd be a good strategy to polish it up and get it merged soon.

I think what's open now is just a couple of small comments above, and rebasing and fixing the current set of conflicts and errors.

…onfig This fixes five errors: /Users/chrisbobbe/dev/zulip-mobile/src/__tests__/reactUtils-test.js 114:3 error Definition for rule 'jest/expect-expect' was not found jest/expect-expect /Users/chrisbobbe/dev/zulip-mobile/src/autocomplete/PeopleAutocomplete.js 96:8 error Definition for rule 'react/jsx-curly-brace-presence' was not found react/jsx-curly-brace-presence /Users/chrisbobbe/dev/zulip-mobile/src/boot/store.js 32:58 error Definition for rule 'import/no-extraneous-dependencies' was not found import/no-extraneous-dependencies /Users/chrisbobbe/dev/zulip-mobile/src/common/ZulipIcon.js 5:1 error Definition for rule 'import/extensions' was not found import/extensions /Users/chrisbobbe/dev/zulip-mobile/src/webview/MessageList.js 45:1 error Definition for rule 'react/no-unused-prop-types' was not found react/no-unused-prop-types The added config reportedly slows down `tools/fmt` by 200-300ms, which is long enough to be noticeable. But it was already running at 4.0 seconds, before this…see discussion at zulip#5393 (comment)

Following the eagerly awaited resolution of prettier/prettier-eslint-cli#304 . And adjust our ESLint config so that we can do this with no new ESLint errors or suppressions. After this, we'll un-ignore the rules under "New rules we want", one by one, which will mean fixing the code that doesn't yet follow each rule. Greg says, about the formatting changes: zulip#5393 (review) > The formatting changes from the Prettier upgrade look fine. Some > code gets nicer (like `foo.bar().baz()` going on one line), some > gets less nice (like a lot of stuff getting an extra level of > indentation). I'm not sure I like it better on net; but Prettier > is designed as a take-it-or-leave-it package, and I don't have a > better alternative nor want to be stuck on an old version. So > 🤷 The added @babel/* deps are to satisfy peer-dependency requirements. The resolutions line gets us prettier/prettier-eslint#749 and can be removed when prettier-eslint-cli bumps its prettier-eslint version.

chrisbobbe · 2022-06-08T03:43:37Z

Thanks for the review! Revision pushed. Still a draft because CI will fail…tomorrow I'll aim to figure out how to make things right for TsFlower.

gnprice · 2022-06-08T16:47:13Z

Hmm, Jest failure too: https://github.com/zulip/zulip-mobile/runs/6786626329?check_suite_focus=true

Looks to be that same flake as at #5401 (comment) . This is atop #5402, so that confirms the latter didn't fix it (though it was still a good change.)

gnprice · 2022-06-08T16:53:17Z

Filed #5404 for the flake. Marked as high-priority, because it seems to be recurring frequently.

gnprice · 2022-06-08T17:00:36Z

For fixing the tsflower suite, what's needed is to update the type definitions so they reflect the new Prettier version (i.e. so they reflect what the result of running tsflower and then auto-formatting would now be, with the new Prettier version), and rebase the patches on top of that.

To do that, I think it suffices to run:

$ tools/tsflower unpack
$ tools/tsflower pack

If tools/tsflower unpack hits rebase conflicts, #5399 describes how to handle them by rerunning Prettier.

gnprice · 2022-06-08T17:04:10Z

To do that, I think it suffices to run:

Which is a compact version of this section of the tools/tsflower help message:

Workflow for updating dependencies, full version:

  1. Update `package.json` and run `yarn` as usual.  Commit
     the results (though the commit may not yet be mergeable.)

  2. Run `tools/tsflower unpack` to unpack patches into a Git
     branch, rerun TsFlower, and attempt to rebase them on the
     updated definitions.

  3. Resolve any rebase conflicts to complete the rebase.

  4. Check that Flow passes; make any needed fixes and commit them.

  5. Run `tools/tsflower pack` as above.

  6. Commit your changes, and squash them into the commit that
     updated `package.json` and `yarn.lock`.

  7. Run `tools/test` to confirm the tests pass.  In particular
     this will run `tools/tsflower check`.

…onfig This fixes five errors: /Users/chrisbobbe/dev/zulip-mobile/src/__tests__/reactUtils-test.js 114:3 error Definition for rule 'jest/expect-expect' was not found jest/expect-expect /Users/chrisbobbe/dev/zulip-mobile/src/autocomplete/PeopleAutocomplete.js 96:8 error Definition for rule 'react/jsx-curly-brace-presence' was not found react/jsx-curly-brace-presence /Users/chrisbobbe/dev/zulip-mobile/src/boot/store.js 32:58 error Definition for rule 'import/no-extraneous-dependencies' was not found import/no-extraneous-dependencies /Users/chrisbobbe/dev/zulip-mobile/src/common/ZulipIcon.js 5:1 error Definition for rule 'import/extensions' was not found import/extensions /Users/chrisbobbe/dev/zulip-mobile/src/webview/MessageList.js 45:1 error Definition for rule 'react/no-unused-prop-types' was not found react/no-unused-prop-types The added config reportedly slows down `tools/fmt` by 200-300ms, which is long enough to be noticeable. But it was already running at 4.0 seconds, before this…see discussion at zulip#5393 (comment)

Following the eagerly awaited resolution of prettier/prettier-eslint-cli#304 . And adjust our ESLint config so that we can do this with no new ESLint errors or suppressions. After this, we'll un-ignore the rules under "New rules we want", one by one, which will mean fixing the code that doesn't yet follow each rule. Greg says, about the formatting changes: zulip#5393 (review) > The formatting changes from the Prettier upgrade look fine. Some > code gets nicer (like `foo.bar().baz()` going on one line), some > gets less nice (like a lot of stuff getting an extra level of > indentation). I'm not sure I like it better on net; but Prettier > is designed as a take-it-or-leave-it package, and I don't have a > better alternative nor want to be stuck on an old version. So > 🤷 The added @babel/* deps are to satisfy peer-dependency requirements. The resolutions line gets us prettier/prettier-eslint#749 and can be removed when prettier-eslint-cli bumps its prettier-eslint version.

Fixes: zulip#4254

Following the eagerly awaited resolution of prettier/prettier-eslint-cli#304 . And adjust our ESLint config so that we can do this with no new ESLint errors or suppressions. After this, we'll un-ignore the rules under "New rules we want", one by one, which will mean fixing the code that doesn't yet follow each rule. Greg says, about the formatting changes: zulip#5393 (review) > The formatting changes from the Prettier upgrade look fine. Some > code gets nicer (like `foo.bar().baz()` going on one line), some > gets less nice (like a lot of stuff getting an extra level of > indentation). I'm not sure I like it better on net; but Prettier > is designed as a take-it-or-leave-it package, and I don't have a > better alternative nor want to be stuck on an old version. So > 🤷 The added @babel/* deps are to satisfy peer-dependency requirements. The resolutions line gets us prettier/prettier-eslint#749 and can be removed when prettier-eslint-cli bumps its prettier-eslint version. And run $ tools/tsflower unpack $ tools/tsflower pack to update TsFlower's output for the new Prettier version and rebase the patches atop that. I see some churn in the patches where it looks like a Git version is printed at the bottom, and mine differs from Greg's, hmm.

See doc at https://eslint.org/docs/user-guide/command-line-interface#--report-unused-disable-directives After enabling this, I ran `tools/test lint --all-files --fix`, then looked at each change and fixed up small things like removing comments on the now-gone eslint-ignores.

Soon, we'll enable the default-param-last ESLint rule.

And add eslint-ignores on our reducers, where we're following a pattern recommended by Redux, which makes things less verbose: https://redux.js.org/usage/structuring-reducers/initializing-state See doc: https://eslint.org/docs/rules/default-param-last

See doc: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/function-component-definition.md

See doc: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-no-useless-fragment.md

See doc: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-arrow-function-lifecycle.md

See doc: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-import-module-exports.md

See doc: https://eslint.org/docs/rules/prefer-regex-literals

And remove some unused React context invocations that this catches. See doc: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-unused-class-component-methods.md

…props

gnprice · 2022-06-09T20:47:56Z

Merged. Excited to have this big swath of dependencies so much more up to date!

chrisbobbe · 2022-06-09T20:50:56Z

Thanks again!

gnprice · 2022-06-09T20:53:04Z

One thing this gives us is the ability to use Flow indexed-access type syntax, as foretold here: #5398 (comment)

I'll go quick make a PR to switch to that (quick, but after lunch.)

Such a nicer syntax! This is available to us now that we've upgraded Prettier: zulip#5393 (comment) and Flow before that: zulip#5398 (comment) We'll similarly convert $ElementType in the next commit. Done mostly with a crude search-and-replace: $ perl -i -0pe ' s/\$PropertyType<(.*?),\s*(.*?)>/${1}[$2]/gs ' src/**/*.js types/**.js.flow flow-typed/**/*.js That doesn't behave right in the presence of nesting; but there was only one case of that, so just fixed it up by hand.

When Chris today first made a change that called for refreshing these patches, after the first few revisions to them had all been done on my machine, we learned that `git format-patch` adds this trailer line with the Git version, causing churn: zulip@b202178bb zulip#5393 (comment) Normalize that line so that it stops causing churn in the future.

As planned; see discussion: zulip#5393 (comment) With allowAsProps: true, it turns out that none of our code is actually flagged as problematic. See doc: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-unstable-nested-components.md

As planned; see discussion: #5393 (comment) With allowAsProps: true, it turns out that none of our code is actually flagged as problematic. See doc: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-unstable-nested-components.md

chrisbobbe requested a review from gnprice May 26, 2022 02:21

gnprice reviewed May 27, 2022

View reviewed changes

chrisbobbe force-pushed the wip-prettier-eslint branch from b79128d to bcab5b6 Compare June 2, 2022 22:42

chrisbobbe force-pushed the wip-prettier-eslint branch from bcab5b6 to 8a7286f Compare June 2, 2022 23:09

chrisbobbe force-pushed the wip-prettier-eslint branch from 8a7286f to 883e1b0 Compare June 3, 2022 18:47

gnprice mentioned this pull request Jun 7, 2022

Show custom profile fields in users' profiles #5398

Merged

gnprice reviewed Jun 7, 2022

View reviewed changes

chrisbobbe force-pushed the wip-prettier-eslint branch from 883e1b0 to 45864a8 Compare June 8, 2022 03:42

gnprice mentioned this pull request Jun 8, 2022

Flake in waitingPeriodThreshold tests #5404

Closed

chrisbobbe added 14 commits June 9, 2022 13:40

eslint: Take babel-eslint's move to @babel/eslint-parser

e183358

Fixes: zulip#4254

js [nfc]: Remove some useless param defaulting

b2bd4b2

Soon, we'll enable the default-param-last ESLint rule.

eslint: Enable default-param-last

3e196c1

And add eslint-ignores on our reducers, where we're following a pattern recommended by Redux, which makes things less verbose: https://redux.js.org/usage/structuring-reducers/initializing-state See doc: https://eslint.org/docs/rules/default-param-last

eslint: Enable react/function-component-definition

a8c0a99

See doc: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/function-component-definition.md

eslint: Enable react/jsx-no-useless-fragment

7006558

See doc: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-no-useless-fragment.md

eslint: Enable react/no-arrow-function-lifecycle

6279515

See doc: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-arrow-function-lifecycle.md

eslint: Enable import/no-import-module-exports

106f109

See doc: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-import-module-exports.md

eslint: Enable prefer-regex-literals

7d11100

See doc: https://eslint.org/docs/rules/prefer-regex-literals

eslint: Enable react/no-unused-class-component-methods

4fa2418

And remove some unused React context invocations that this catches. See doc: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-unused-class-component-methods.md

MessageReactionsScreen [nfc]: Convert to function component

c1d2702

MessageReactionsScreen [nfc]: Move up destructuring assignments from …

9d8ff51

…props

MessageReactionsScreen [nfc]: Use useSelector instead of connect

dab0850

gnprice force-pushed the wip-prettier-eslint branch from 8a82b7f to dab0850 Compare June 9, 2022 20:43

gnprice merged commit dab0850 into zulip:main Jun 9, 2022

chrisbobbe deleted the wip-prettier-eslint branch June 9, 2022 20:50

gnprice mentioned this pull request Jun 9, 2022

flow [nfc]: Convert $PropertyType and $ElementType to indexed-access syntax #5406

Merged

gnprice mentioned this pull request Jun 9, 2022

tsflower: Make patches more reproducible between machines/developers #5407

Merged

chrisbobbe mentioned this pull request Jun 24, 2022

events: Remove brittle needsInitialFetch system #5300

Merged

chrisbobbe mentioned this pull request Jul 13, 2022

eslint: Enable react/no-unstable-nested-components #5440

Merged

		@@ -1,4 +1,5 @@
		/* @flow strict-local */
		/* eslint-disable no-shadow */

		--eslint-config-path ./tools/formatting.eslintrc.yaml \
		--eslint-config-path $root_dir/tools/formatting.eslintrc.yaml \

js: Update prettier-eslint, etc., to latest!! #5393

js: Update prettier-eslint, etc., to latest!! #5393

Conversation

chrisbobbe commented May 26, 2022

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jun 3, 2022

chrisbobbe commented Jun 3, 2022 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Jun 7, 2022

chrisbobbe commented Jun 7, 2022

gnprice commented Jun 7, 2022 • edited Loading

gnprice commented Jun 7, 2022

gnprice commented Jun 7, 2022

chrisbobbe commented Jun 8, 2022

gnprice commented Jun 8, 2022

gnprice commented Jun 8, 2022

gnprice commented Jun 8, 2022

gnprice commented Jun 8, 2022

gnprice commented Jun 9, 2022

chrisbobbe commented Jun 9, 2022

gnprice commented Jun 9, 2022

chrisbobbe commented Jun 3, 2022 •

edited

Loading

chrisbobbe Jun 7, 2022 •

edited

Loading

chrisbobbe Jun 7, 2022 •

edited

Loading

gnprice commented Jun 7, 2022 •

edited

Loading