Prep for initial-fetch timeout. #4226
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gnprice
reviewed
Aug 13, 2020
There was a problem hiding this comment.
Thanks @chrisbobbe ! These all look great modulo the small remark below. Feel free to merge after fixing that.
package.json
Outdated
| @@ -32,6 +32,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "@expo/react-native-action-sheet": "^3.4.0", | |||
| "@jest/source-map": "^26.1.0", | |||
There was a problem hiding this comment.
This can go in devDependencies, alongside the other Jest-related packages.
(Discussion here apropos of a different recent change.)
A reversion of e40c020 and 62621ef. Unfortunately, Jest 26 (the latest), which we'll take in an upcoming commit, doesn't support `jest-expo` at 37 [1]. We can't go past 37 (e.g., to 38.0.2, the latest) until we upgrade to React Native v0.62 (that's zulip#3782). That's because jest-expo's "react" peer dependency was bumped from ~16.9.0 to ~16.11.0 in expo/expo#8310 [2], and we must do that React upgrade atomically with our RN upgrade. The "react-native" peer dependency wasn't touched; it remained at "*". So, I'm left unsure whether it was intentional to drop support for RN v0.61 [3]. Ah, well. As I said in e40c020: """ If `jest-expo` turns out to be buggy, or the dependency requirements get even more tangled or burdensome, we should feel free to abandon this effort; it's not terrible to have to add boring mocks. """ We may consider adding it back in as a followup to zulip#3782. Run our usual `yarn yarn-deduplicate && yarn`, as prompted by `tools/test deps`. [1] We get errors about jest-expo using `require.requireActual`, which was removed in jestjs/jest#9854, out in v26.0.0-alpha.0. [2] expo/expo@a4cabf30a#diff-4a85ebd1069aff25ee2e5f2b004281ccR33 [3] See react-native-webview/react-native-webview#1445.
Also, add @jest/source-map, without which some chain of dependencies doesn't go the right way, and the "modern" fake-timer implementation isn't available. (That landed in jestjs/jest#7776 with jestjs/jest@71631f6bf.) The libdef process was fairly smooth: there was one available from FlowTyped, so I ran `flow-typed install jest@26`. Add a line in our Flow config under [libs] as suggested in chat [1]; otherwise, Flow sees a Jest libdef in `node_modules/react-native` and uses that instead. In an upcoming commit, we'll add a few methods on the Jest object that didn't make it into the libdef in FlowTyped for whatever reason. Run `yarn yarn-deduplicate && yarn` as prompted by `tools/test deps`. [1] https://chat.zulip.org/#narrow/stream/48-mobile/topic/flow-typed.20for.20Jest/near/854562
These are reflected in the doc [1] [2] and in the `Jest` interface in `node_modules/@jest/environment/build/index.d.ts`. A small difference in the comments: the doc says "modern fake timers implementation", where the TypeScript says "Lolex as fake timers implementation". I've sided with the docs, as it's probably wise to treat Lolex as an implementation detail of "modern" fake timers that may change. Adjust the entry under `[libs]` as Ray suggests in chat [3]. [1] https://jestjs.io/docs/en/jest-object#jestgetrealsystemtime [2] https://jestjs.io/docs/en/jest-object#jestsetsystemtimenow-number--date [3] https://chat.zulip.org/#narrow/stream/48-mobile/topic/flow-typed.20for.20Jest/near/854562
As Greg points out [1], about the current (subtly incorrect) form: """ I believe the semantics of this is actually that that [`methodName = …`] becomes part of the constructor! Not a method on the class. """ We want methods on the class, so, write them that way (as "method definitions") [2]. In particular, we plan to implement and write tests for a new `tryFetch` implementation that retries a network request until a timeout. In the tests, we'll want to mock `BackoffMachine.prototype.wait` so we don't have to think about the special things it does. (BackoffMachine has its own unit tests, after all.) Making `wait` an actual method on the class will let us accomplish this straightforwardly. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/909006 [2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions
`tryUntilSuccessful` is about to grow a timeout, which we've had in mind in particular for the initial fetch. It's also about to be moved into this file, and we want to keep its association with `doInitialFetch` by putting it directly above it, with nothing in between. Removing this call site lets us do so without ESLint complaining of `no-use-before-define`. And Greg says it's all the same to him [1], especially since `fetchPrivateMessages` is doomed to be deleted anyway, as the JSDoc says. [1]: zulip#4165 (comment)
As Greg says [1], it's always been closely tied to `doInitialFetch`'s specific needs, with its handling of 4xx errors. We're about to make it more so, by adding timeout logic. [1]: zulip#4165 (comment)
As hinted by the comment above this edit, even an empty array is incomplete server data: "Valid server data must have a user: the self user, at a minimum.". But the initial state of `users` (`initialState` in src/users/usersReducer.js) is an empty array. LOGOUT, LOGIN_SUCCESS, and ACCOUNT_SWITCH all set `users` to that initial state. Those three actions are handled in `navReducer` by navigating to routes that don't depend on complete server data: 'loading' or 'account'. But if we quit the app with `users` having this initial state (e.g., giving up in frustration while trying to connect to a realm for a long time), we should be sure that, on reopening it and rehydrating, we don't try to go to 'main' before that initial state has been replaced with a complete state. Otherwise, we see "No server data found" errors and a white screen. See also discussion [1]. [1]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4166.20Time.20out.20initial.20fetch/near/912017
chrisbobbe
force-pushed
the
pr-prep-initial-fetch-timeout
branch
from
647ed53 to
a1fab9b
Compare
|
Thanks for the review! Just merged, after moving that dependency to I ran |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This work helps prepare for fixing #4165.
Work on this issue started at #4166, then we ran into a bunch of issues with getting the tests to work. #4193 is currently open as a draft, for discussion about how to fix those; Greg and I had some of that discussion over the phone, and it continues on CZO here.
These are some commits from #4193 that aren't being held up by anything, so they're ready for review. They don't fix #4165 at all, but they do prepare for us to get working (and not absurd-looking) tests.