Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools/test: Fix regression that disabled ESLint #5569

Merged
merged 8 commits into from
Nov 30, 2022
Merged

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Nov 23, 2022

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, 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:

/home/greg/z/mobile/src/action-sheets/index.js
  47:1  error  Dependency cycle via ../autocomplete/AutocompleteView:66=>./StreamAutocomplete:9=>../streams/StreamItem:11  import/no-cycle

/home/greg/z/mobile/src/autocomplete/AutocompleteView.js
  9:1  error  Dependency cycle via ../streams/StreamItem:11=>../action-sheets:9=>../compose/ComposeBox:47  import/no-cycle

/home/greg/z/mobile/src/autocomplete/StreamAutocomplete.js
  11:1  error  Dependency cycle via ../action-sheets:9=>../compose/ComposeBox:47=>../autocomplete/AutocompleteView:66  import/no-cycle

/home/greg/z/mobile/src/chat/ChatScreen.js
  2:42  warning  'useState' is defined but never used  no-unused-vars

/home/greg/z/mobile/src/compose/ComposeBox.js
  66:1  error  Dependency cycle via ./StreamAutocomplete:9=>../streams/StreamItem:11=>../action-sheets:9  import/no-cycle

/home/greg/z/mobile/src/streams/StreamItem.js
  9:1  error  Dependency cycle via ../compose/ComposeBox:47=>../autocomplete/AutocompleteView:66=>./StreamAutocomplete:9  import/no-cycle

/home/greg/z/mobile/src/webview/MessageList.js
  323:5  warning  React Hook React.useCallback has a missing dependency: 'navigation'. Either include it or remove the dependency array  react-hooks/exhaustive-deps

✖ 7 problems (5 errors, 2 warnings)

So those will need to be fixed first.

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Nov 23, 2022

Ooh, thanks for catching this and for the fix!

My branch lint-regression-fixes has fixes for those regressions in main; I've just pushed that branch.

@gnprice
Copy link
Member Author

gnprice commented Nov 30, 2022

My branch lint-regression-fixes has fixes for those regressions in main; I've just pushed that branch.

Thanks! Rebased atop those and pushed here.

Also filed #5574 for the issue, to point to in commit messages.

Comment on lines 97 to 99
composeBoxRefCurrent: React$ElementRef<ComposeBox> | null,
composeBoxRefCurrent: {
+doQuoteAndReply: (message: Message | Outbox) => Promise<void>,
...
} | null,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

chrisbobbe and others added 8 commits November 30, 2022 13:23
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.
@chrisbobbe
Copy link
Contributor

Thanks for the review! Revision pushed.

@gnprice
Copy link
Member Author

gnprice commented Nov 30, 2022

Looks good, thanks! Merging.

@gnprice gnprice marked this pull request as ready for review November 30, 2022 22:33
@gnprice gnprice merged commit be50892 into zulip:main Nov 30, 2022
@gnprice gnprice deleted the pr-lint branch November 30, 2022 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tools/test lint always passes
2 participants