tools/test: Fix regression that disabled ESLint #5569
Conversation
|
Ooh, thanks for catching this and for the fix! My branch |
Thanks! Rebased atop those and pushed here. Also filed #5574 for the issue, to point to in commit messages. |
src/action-sheets/index.js
Outdated
| composeBoxRefCurrent: React$ElementRef<ComposeBox> | null, | ||
| composeBoxRefCurrent: { | ||
| +doQuoteAndReply: (message: Message | Outbox) => Promise<void>, | ||
| ... | ||
| } | null, |
There was a problem hiding this comment.
Hmm, this seems like the code gets less readable than it was.
Looking up at the import statement that gets cut:
import typeof ComposeBox from '../compose/ComposeBox';that's a type-only import. The lint rule isn't supposed to apply to those, as mentioned in tools/import-cycles:
# Tool to explore import cycles, as seen by Flow.
#
# […]
#
# NB these cycles generally do *not* mean an import cycle in the
# transpiled runtime code. In fact, we use ESLint to prohibit those,
# with rule import/no-cycle.
I suspect it has a bug where it recognizes import type but not import typeof, and accidentally treats the latter the same as plain import.
There was a problem hiding this comment.
Kind of annoying to handle via eslint-disable, though, because it's reported on each import in the cycle and so would need such a line on each one of them, most of which are perfectly real runtime imports.
It looks like the import in MessageList isn't part of a cycle. So, perhaps acquiesce to the buggy rule here in action-sheets/, but leave MessageList clean?
There was a problem hiding this comment.
Or, in place of React$ElementRef<typeof ComposeBox>, we could use the ImperativeHandle type in ComposeBox.js? I'll try that in my next revision and you can see what you think.
There was a problem hiding this comment.
OK, and meanwhile sent import-js/eslint-plugin-import#2608 to the relevant ESLint plugin, so hopefully that gets merged and we get to forget about this constraint in the future.
This fixes the following lint errors (formatted to fit better in a commit message) that slipped through because of zulip#5574: /Users/chrisbobbe/dev/zulip-mobile/src/action-sheets/index.js 47:1 error import/no-cycle Dependency cycle via ../autocomplete/AutocompleteView:66=>./StreamAutocomplete:9=>../streams/StreamItem:11 /Users/chrisbobbe/dev/zulip-mobile/src/autocomplete/AutocompleteView.js 9:1 error import/no-cycle Dependency cycle via ../streams/StreamItem:11=>../action-sheets:9=>../compose/ComposeBox:47 /Users/chrisbobbe/dev/zulip-mobile/src/autocomplete/StreamAutocomplete.js 11:1 error import/no-cycle Dependency cycle via ../action-sheets:9=>../compose/ComposeBox:47=>../autocomplete/AutocompleteView:66 /Users/chrisbobbe/dev/zulip-mobile/src/compose/ComposeBox.js 66:1 error import/no-cycle Dependency cycle via ./StreamAutocomplete:9=>../streams/StreamItem:11=>../action-sheets:9 /Users/chrisbobbe/dev/zulip-mobile/src/streams/StreamItem.js 9:1 error import/no-cycle Dependency cycle via ../compose/ComposeBox:47=>../autocomplete/AutocompleteView:66=>./StreamAutocomplete:9 Odd that we get these errors, because as Greg points out the import/no-cycle rule isn't supposed to apply to type-only imports; see zulip#5569 (comment) . As mentioned there, we suspect it has a bug where it recognizes `import type` but not `import typeof`, and accidentally treats the latter the same as plain `import`. Anyway, to fix, rearrange to use an `import type` instead of an `import typeof`. The use of this ImperativeHandle type seems OK and doesn't go against its jsdoc. ( We couldn't do `import type ComposeBox from …` because Flow would complain: Cannot import the default export as a type. `import type` only works on type exports like type aliases, interfaces, and classes. If you intended to import the type of a value use `import typeof` instead. Flow(import-value-as-type) )
This fixes the following lint error that slipped through because of issue zulip#5574: /Users/chrisbobbe/dev/zulip-mobile/src/chat/ChatScreen.js 2:42 warning 'useState' is defined but never used no-unused-vars
This fixes the following lint error (formatted to fit better in a commit message) that slipped through because of zulip#5574: /Users/chrisbobbe/dev/zulip-mobile/src/webview/MessageList.js 327:5 warning react-hooks/exhaustive-deps React Hook React.useCallback has a missing dependency: 'navigation'. Either include it or remove the dependency array
We've had the name "files" for both a global representing the user's choice of which files to operate on, and a local inside the run_lint function. This collision was there for a long time without causing a live problem, and then started doing so with f3aad7e where we moved a function call that consulted the global so that it occurred inside run_lint, with the local active. (Bash "local" variables are only sort of local -- they're dynamically scoped, rather than lexically scoped.) The effect of the issue is that the "lint" suite would always succeed without checking anything, because `files_js` would always produce no output. Fix it by giving the global a longer, more explicit name. Fixes: zulip#5574
For consistency with the just-renamed opt_files, and in order to help prevent the same kind of bug occurring here by giving this global a longer, more explicit name too.
|
Thanks for the review! Revision pushed. |
|
Looks good, thanks! Merging. |
We've had the name "files" for both a global representing the
user's choice of which files to operate on, and a local inside
the run_lint function.
This collision was there for a long time without causing a live
problem, and then started doing so with f3aad7e where we moved
a function call that consulted the global so that it occurred
inside run_lint, with the local active. (Bash "local" variables
are only sort of local -- they're dynamically scoped, rather than
lexically scoped.)
The effect of the issue is that the "lint" suite would always
succeed without checking anything, because
files_jswouldalways produce no output.
Fix it by giving the global a longer, more explicit name,
opt_files.Also rename other options globals similarly.
Fixes: #5574
This PR is a draft because we have some lint errors that snuck in since this regression:
So those will need to be fixed first.