From 54c95671ca22a3896502027a5ea4b92dcfcdf054 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 22 Nov 2022 16:50:38 -0800 Subject: [PATCH 1/8] lint: Fix regression in import/no-cycle (ComposeBox) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes the following lint errors (formatted to fit better in a commit message) that slipped through because of #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 https://github.com/zulip/zulip-mobile/pull/5569#discussion_r1036428798 . 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) ) --- src/action-sheets/index.js | 14 +++++++------- src/chat/ChatScreen.js | 6 ++++-- src/compose/ComposeBox.js | 2 +- src/webview/MessageList.js | 4 ++-- src/webview/handleOutboundEvents.js | 2 +- 5 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/action-sheets/index.js b/src/action-sheets/index.js index 8a91ab6b8c..7bc8fff573 100644 --- a/src/action-sheets/index.js +++ b/src/action-sheets/index.js @@ -44,7 +44,7 @@ import { Role, type RoleT } from '../api/permissionsTypes'; import { roleIsAtLeast } from '../permissionSelectors'; import { kNotificationBotEmail } from '../api/constants'; import type { AppNavigationMethods } from '../nav/AppNavigator'; -import typeof ComposeBox from '../compose/ComposeBox'; +import { type ImperativeHandle as ComposeBoxImperativeHandle } from '../compose/ComposeBox'; // TODO really this belongs in a libdef. export type ShowActionSheetWithOptions = ( @@ -94,7 +94,7 @@ type MessageArgs = { dispatch: Dispatch, startEditMessage: (editMessage: EditMessage) => void, setDoNotMarkMessagesAsRead: boolean => void, - composeBoxRefCurrent: React$ElementRef | null, + composeBoxImperativeHandle: ComposeBoxImperativeHandle | null, navigation: AppNavigationMethods, _: GetText, ... @@ -133,12 +133,12 @@ const reply = { const quoteAndReply = { title: 'Quote and reply', errorMessage: 'Quote-and-reply failed', - action: async ({ message, composeBoxRefCurrent }) => { - if (!composeBoxRefCurrent) { + action: async ({ message, composeBoxImperativeHandle }) => { + if (!composeBoxImperativeHandle) { logging.error("quoteAndReply button pressed when it shouldn't have appeared in the UI"); return; } - return composeBoxRefCurrent.doQuoteAndReply(message); + return composeBoxImperativeHandle.doQuoteAndReply(message); }, }; @@ -807,7 +807,7 @@ export const showMessageActionSheet = (args: {| callbacks: {| dispatch: Dispatch, startEditMessage: (editMessage: EditMessage) => void, - composeBoxRefCurrent: React$ElementRef | null, + composeBoxImperativeHandle: ComposeBoxImperativeHandle | null, navigation: AppNavigationMethods, _: GetText, setDoNotMarkMessagesAsRead: boolean => void, @@ -833,7 +833,7 @@ export const showMessageActionSheet = (args: {| backgroundData, message, narrow, - canStartQuoteAndReply: callbacks.composeBoxRefCurrent !== null, + canStartQuoteAndReply: callbacks.composeBoxImperativeHandle !== null, }), args: { ...backgroundData, ...callbacks, message, narrow }, }); diff --git a/src/chat/ChatScreen.js b/src/chat/ChatScreen.js index 8b85454f0c..bcb18ad436 100644 --- a/src/chat/ChatScreen.js +++ b/src/chat/ChatScreen.js @@ -15,7 +15,9 @@ import NoMessages from '../message/NoMessages'; import FetchError from './FetchError'; import InvalidNarrow from './InvalidNarrow'; import { fetchMessagesInNarrow } from '../message/fetchActions'; -import ComposeBox from '../compose/ComposeBox'; +import ComposeBox, { + type ImperativeHandle as ComposeBoxImperativeHandle, +} from '../compose/ComposeBox'; import UnreadNotice from './UnreadNotice'; import { showComposeBoxOnNarrow, caseNarrowDefault, keyFromNarrow } from '../utils/narrow'; import { getLoading, getSession } from '../directSelectors'; @@ -144,7 +146,7 @@ export default function ChatScreen(props: Props): Node { const sayNoMessages = messages.length === 0 && !isFetching; const showComposeBox = showComposeBoxOnNarrow(narrow) && !showMessagePlaceholders; - const composeBoxRef = React.useRef | null>(null); + const composeBoxRef = React.useRef(null); const auth = useSelector(getAuth); const dispatch = useDispatch(); diff --git a/src/compose/ComposeBox.js b/src/compose/ComposeBox.js index 77dd454753..880412b18b 100644 --- a/src/compose/ComposeBox.js +++ b/src/compose/ComposeBox.js @@ -111,7 +111,7 @@ export type ValidationError = /** * Functions expected to be called using a ref to this component. */ -type ImperativeHandle = {| +export type ImperativeHandle = {| /** * Take a message ID, fetch its raw Markdown content, and put it in the * compose box with proper formatting. diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 74b8c1a00a..2722fcbf77 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -41,7 +41,7 @@ import { type BackgroundData, getBackgroundData } from './backgroundData'; import { ensureUnreachable } from '../generics'; import SinglePageWebView from './SinglePageWebView'; import { usePrevious } from '../reactUtils'; -import typeof ComposeBox from '../compose/ComposeBox'; +import { type ImperativeHandle as ComposeBoxImperativeHandle } from '../compose/ComposeBox'; /** * The actual React props for the MessageList component. @@ -54,7 +54,7 @@ type OuterProps = $ReadOnly<{| startEditMessage: (editMessage: EditMessage) => void, // Careful: We expect this prop to be mutable, which is unusual. - composeBoxRef: {| current: React$ElementRef | null |}, + composeBoxRef: {| current: ComposeBoxImperativeHandle | null |}, |}>; /** diff --git a/src/webview/handleOutboundEvents.js b/src/webview/handleOutboundEvents.js index 1dcb5fff29..c4f00cf0b6 100644 --- a/src/webview/handleOutboundEvents.js +++ b/src/webview/handleOutboundEvents.js @@ -258,7 +258,7 @@ const handleLongPress = (args: {| // that an action-sheet button press will act on values that were // current when the action sheet was opened: // https://github.com/zulip/zulip-mobile/pull/5554#discussion_r1027004559 - composeBoxRefCurrent: composeBoxRef.current, + composeBoxImperativeHandle: composeBoxRef.current, navigation, _, From b9009ed937cb274e6b6e38aa50e6ba4ca70ba063 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 22 Nov 2022 16:56:02 -0800 Subject: [PATCH 2/8] lint: Fix regression in no-unused-vars (useState in ChatScreen) This fixes the following lint error that slipped through because of issue #5574: /Users/chrisbobbe/dev/zulip-mobile/src/chat/ChatScreen.js 2:42 warning 'useState' is defined but never used no-unused-vars --- src/chat/ChatScreen.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/chat/ChatScreen.js b/src/chat/ChatScreen.js index bcb18ad436..7e6af9438b 100644 --- a/src/chat/ChatScreen.js +++ b/src/chat/ChatScreen.js @@ -1,5 +1,5 @@ /* @flow strict-local */ -import React, { useCallback, useContext, useState } from 'react'; +import React, { useCallback, useContext } from 'react'; import type { Node } from 'react'; import { useIsFocused } from '@react-navigation/native'; From 715cfc26910b599f5812ac8657d0d5acaa8f7671 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 22 Nov 2022 16:56:43 -0800 Subject: [PATCH 3/8] lint: Fix regression in react-hooks/exhaustive-deps (MessageList) This fixes the following lint error (formatted to fit better in a commit message) that slipped through because of #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 --- src/webview/MessageList.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 2722fcbf77..5e16a335f6 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -320,7 +320,7 @@ export default function MessageList(outerProps: OuterProps): React.Node { handleWebViewOutboundEvent(propsRef.current, navigation, eventData); } }, - [sendInboundEvents], + [sendInboundEvents, navigation], ); // We compute the page contents as an HTML string just once (*), on this From bb7f700319fd5bbf53e47c8b3889dee1c874a663 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 22 Nov 2022 15:30:30 -0800 Subject: [PATCH 4/8] tools/test: Fix name collision on "files", saying "opt_files" 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 f3aad7ea2 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: #5574 --- tools/test | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tools/test b/tools/test index b60c8c7d7d..ae74782fec 100755 --- a/tools/test +++ b/tools/test @@ -69,15 +69,15 @@ EOF } coverage= -files=branch +opt_files=branch platform=sloppy fix= suites=() while (( $# )); do case "$1" in --coverage) coverage=1; shift;; - --diff) shift; files=diff:"$1"; shift;; - --all-files) files=all; shift;; + --diff) shift; opt_files=diff:"$1"; shift;; + --all-files) opt_files=all; shift;; --platform) shift; case "$1" in @@ -86,7 +86,7 @@ while (( $# )); do esac shift ;; - --all) files=all; platform=both; shift;; + --all) opt_files=all; platform=both; shift;; --fix) fix=1; shift;; native|flow|lint|jest|prettier|deps|tsflower) suites+=("$1"); shift;; @@ -101,10 +101,10 @@ if [ -z "$suites" ]; then fi files_base_commit= -case "$files" in +case "$opt_files" in all) ;; branch) files_base_commit="$(tools/git base)";; - diff:*) files_base_commit="${files#diff:}";; + diff:*) files_base_commit="${opt_files#diff:}";; esac @@ -115,12 +115,12 @@ cd "$rootdir" PATH=node_modules/.bin:"$PATH" -# Intersect $files with the set of our JS files in src/. +# Intersect $opt_files with the set of our JS files in src/. # # Prints a list of newline-terminated paths; either files, or # directories meaning their whole subtrees. files_js() { - case "$files" in + case "$opt_files" in all) echo src/ ;; @@ -132,7 +132,7 @@ files_js() { # True just if $files intersects the given set of paths. files_check() { - case "$files" in + case "$opt_files" in all) ;; branch | diff:*) @@ -189,9 +189,9 @@ run_lint() { } run_jest() { - # Unlike some others, this inspects "$files" for itself. + # Unlike some others, this inspects "$opt_files" for itself. local jest_args=() - case "$files" in + case "$opt_files" in all) if [ -n "$coverage" ]; then jest_args+=( --coverage ) @@ -225,7 +225,7 @@ run_jest() { run_prettier() { local patterns - case "$files" in + case "$opt_files" in all) # The prettier-eslint CLI won't take directories; # but it's happy to take glob patterns, and supports `**`. From c1597596880afdab5b4c35963fb17793d10ee254 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 22 Nov 2022 15:29:08 -0800 Subject: [PATCH 5/8] tools/test [nfc]: Rename opt_coverage from plain "coverage" 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. --- tools/test | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/test b/tools/test index ae74782fec..5d705f251a 100755 --- a/tools/test +++ b/tools/test @@ -68,14 +68,14 @@ EOF exit 2 } -coverage= +opt_coverage= opt_files=branch platform=sloppy fix= suites=() while (( $# )); do case "$1" in - --coverage) coverage=1; shift;; + --coverage) opt_coverage=1; shift;; --diff) shift; opt_files=diff:"$1"; shift;; --all-files) opt_files=all; shift;; --platform) @@ -193,7 +193,7 @@ run_jest() { local jest_args=() case "$opt_files" in all) - if [ -n "$coverage" ]; then + if [ -n "$opt_coverage" ]; then jest_args+=( --coverage ) fi ;; From 355cac801fe9cafa669fe6e3526ef151f380999d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 22 Nov 2022 15:35:13 -0800 Subject: [PATCH 6/8] tools/test [nfc]: Rename opt_platform from plain "platform" --- tools/test | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/test b/tools/test index 5d705f251a..0a7e502c96 100755 --- a/tools/test +++ b/tools/test @@ -70,7 +70,7 @@ EOF opt_coverage= opt_files=branch -platform=sloppy +opt_platform=sloppy fix= suites=() while (( $# )); do @@ -81,12 +81,12 @@ while (( $# )); do --platform) shift; case "$1" in - ios|android|both|sloppy) platform="$1";; + ios|android|both|sloppy) opt_platform="$1";; *) usage;; esac shift ;; - --all) opt_files=all; platform=both; shift;; + --all) opt_files=all; opt_platform=both; shift;; --fix) fix=1; shift;; native|flow|lint|jest|prettier|deps|tsflower) suites+=("$1"); shift;; @@ -166,12 +166,12 @@ run_native_ios() { } run_native() { - if [[ $platform == android || $platform == both || $platform == sloppy ]]; then + if [[ $opt_platform == android || $opt_platform == both || $opt_platform == sloppy ]]; then echo 'Running Android native tests...'; run_native_android || return fi - if [[ $platform == ios || $platform == both || $platform == sloppy ]]; then + if [[ $opt_platform == ios || $opt_platform == both || $opt_platform == sloppy ]]; then # TODO: Run if on macOS; otherwise, echo that these tests are # skipped because they can't be run. @@ -209,7 +209,7 @@ run_jest() { esac local platforms=( ios android ) - case "$platform" in + case "$opt_platform" in ios) jest_args+=( --selectProjects ios );; android) jest_args+=( --selectProjects android );; both) jest_args+=( --selectProjects ios android );; From 3954972e7d4e5a199ce7910386f3e8f540aa75dd Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 22 Nov 2022 15:35:59 -0800 Subject: [PATCH 7/8] tools/test [nfc]: Rename opt_fix from plain "fix" --- tools/test | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/test b/tools/test index 0a7e502c96..8a6cca044b 100755 --- a/tools/test +++ b/tools/test @@ -71,7 +71,7 @@ EOF opt_coverage= opt_files=branch opt_platform=sloppy -fix= +opt_fix= suites=() while (( $# )); do case "$1" in @@ -87,7 +87,7 @@ while (( $# )); do shift ;; --all) opt_files=all; opt_platform=both; shift;; - --fix) fix=1; shift;; + --fix) opt_fix=1; shift;; native|flow|lint|jest|prettier|deps|tsflower) suites+=("$1"); shift;; *) usage;; @@ -185,7 +185,7 @@ run_lint() { files=( $(files_js) ) files=( $(apply_eslintignore "${files[@]}") ) (( ${#files[@]} )) || return 0 - eslint ${fix:+--fix} --report-unused-disable-directives --max-warnings=0 "${files[@]}" + eslint ${opt_fix:+--fix} --report-unused-disable-directives --max-warnings=0 "${files[@]}" } run_jest() { @@ -242,7 +242,7 @@ run_prettier() { # Workaround for https://github.com/prettier/prettier-eslint-cli/issues/205 patterns=( "${patterns[@]/#/$rootdir/}" ) prettier-eslint \ - ${fix:+--write} \ + ${opt_fix:+--write} \ --list-different \ --eslint-config-path "${rootdir}"/tools/formatting.eslintrc.yaml \ "${patterns[@]}" From be508920c995109b045ac0c3fc34207cac6202e6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 22 Nov 2022 15:36:47 -0800 Subject: [PATCH 8/8] tools/test [nfc]: Rename opt_suites from plain "suites" --- tools/test | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/test b/tools/test index 8a6cca044b..7892630445 100755 --- a/tools/test +++ b/tools/test @@ -72,7 +72,7 @@ opt_coverage= opt_files=branch opt_platform=sloppy opt_fix= -suites=() +opt_suites=() while (( $# )); do case "$1" in --coverage) opt_coverage=1; shift;; @@ -89,15 +89,15 @@ while (( $# )); do --all) opt_files=all; opt_platform=both; shift;; --fix) opt_fix=1; shift;; native|flow|lint|jest|prettier|deps|tsflower) - suites+=("$1"); shift;; + opt_suites+=("$1"); shift;; *) usage;; esac done -if [ -z "$suites" ]; then +if [ -z "$opt_suites" ]; then # This default doesn't have to be the complete list; just be sure to # document in the usage message any suites that it skips. - suites=(native flow lint jest prettier deps tsflower) + opt_suites=(native flow lint jest prettier deps tsflower) fi files_base_commit= @@ -285,7 +285,7 @@ run_tsflower() { } failed=() -for suite in "${suites[@]}"; do +for suite in "${opt_suites[@]}"; do echo "Running $suite..." case "$suite" in native) run_native ;;