Add tests for ZulipAsyncStorage.getItem and .setItem.

[chrisbobbe]

And a broad round-tripping test for several pieces of the Redux storage logic at once (sadly, not redux-persist, but we're probably getting rid of that).

And cut out a bunch of src/third/redux-persist that's there to help web apps, IIRC, but does nothing for us.

This is meant to follow #4700 and is based on the tip of my current revision for that. Done!

It should come before #4694, so I'm marking it P1.

[chrisbobbe]

Just pushed a new revision with a small change to the first commit, and a new commit just before that.

[WesleyAC]

Looks good, mostly requests for more comments :)

[WesleyAC]

It's unclear to me that this is an actual improvement — this makes it look like it's intentional that this is iOS only, whereas I think the reality is we just don't have a good way of detecting Android tablets, but if we could, we'd like them to have labels, right?

I don't have a strong feeling on this, but this seems odd to me.

I guess maybe your comment is hinting at needing the Platform.OS check because isPad is undefined on iOS? If that's the case, that makes sense, but I think the commit message could be clearer.

[chrisbobbe]

the reality is we just don't have a good way of detecting Android tablets, but if we could, we'd like them to have labels

That's right, yeah.

Yeah; this could be made clearer—I hopped over here from #4750 (comment) to add to the "flow config: Stop ignoring Android-only JS files" commit, since I realized that that commit might be a good place to add the module.file_ext=.android.js line that's apparently needed for react-native-paper's setup. (And then I could cherry-pick the improved commit over to that other PR if it didn't land here first.) So I wasn't paying close attention to the context; my mind was on react-native-paper.

Specifically, after adding the module.file_ext=.android.js line to the "Stop ignoring Android-only JS files" commit, I get this error if I don't change !!Platform.isPad:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/main/MainTabsScreen.js:55:33

Cannot get Platform.isPad because property isPad is missing in object literal [1]. [prop-missing]

     src/main/MainTabsScreen.js
      52│       <OfflineNotice />
      53│       <Tab.Navigator
      54│         {...bottomTabNavigatorConfig({
      55│           showLabel: !!Platform.isPad,
      56│           showIcon: true,
      57│         })}
      58│         backBehavior="none"

     node_modules/react-native/index.js
 [1] 462│   get Platform(): Platform {



Found 1 error

I think the linked doc is clear that isPad is missing on Android, not iOS; it's marked with an 'iOS' tag. It's less clear in the commit message that, once Flow is made aware that isPad is unavailable on Android (by having it not ignore Android-only JS files), Flow seems to count as unsafe any attempts to access isPad on Android, even if they'd basically be harmless. So this a necessary change, unless we want a $FlowFixMe or similar. I'll say that in the commit message, and I'll also leave a comment that we should keep an eye out for API changes that can let us know if we're on an Android tablet.

[WesleyAC]

Is it correct for this function to both call the callback with the error and throw?

[chrisbobbe]

Yeah; AsyncStorage's methods both return a Promise and cause a callback to be called; I wish they'd stick with Promises. Throwing an error in an async function causes the returned Promise to reject: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function#return_value.

I think this might be the file where it's implemented (but I don't know what it's doing with window, which doesn't naturally exist in React Native; we use global instead):

  static setItem(key: string, value: string, callback?: Function): Promise<*> {
    return createPromise(() => {
      window.localStorage.setItem(key, value);
    }, callback);
  }

and above that,

const createPromise = (getValue, callback): Promise<*> => {
  return new Promise((resolve, reject) => {
    try {
      const value = getValue();
      if (callback) {
        callback(null, value);
      }
      resolve(value);
    } catch (err) {
      if (callback) {
        callback(err);
      }
      reject(err);
    }
  });
};

[WesleyAC]

It feels weird that we need to manually compress the value here, like something is off with our abstraction.

I don't think we need to try to fix that right now, just something to consider.

[chrisbobbe]

There's a different way it could go: we could use ZulipAsyncStorage.setItem itself; that'd take care of the compression. I've chosen not to, in this revision, because that would mean inadvertently and unnecessarily "testing" ZulipAsyncStorage.setItem (without coherent pass/fail output) within the tests that are marked as testing ZulipAsyncStorage.getItem. That choice seemed like a good practice, to me, but it's possible that I'm wrong; what do you think?

(And if it is good practice, I should probably add a comment that it's intentional, right?)

[WesleyAC]

I'm not convinced that this new name is worth going onto multiple lines for, but up to you.

[chrisbobbe]

I bet I can make it clear what item is with an implementation comment instead of a new name.

[WesleyAC]

Cann you add more explanation to the commit message about why you're making these covariant? I read the flow docs on it (which it'd be nice to link to, btw), but that doesn't tell me why you want covariance.

[chrisbobbe]

Yeah, I agree. I've raised a question on CZO here as I'm not sure how best to condense the reason down from 130 Flow errors.

[chrisbobbe]

I think this is resolved by using $ReadOnly, which does the same thing but is clearer about what it does; I've done that in my new revision.

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Thanks for these changes! Glad to have some of the cruft in these layers cleaned up, and to start having tests on this very core code.

Some comments below.

---

Another comment that spans several files and commits is: instead of having a .js and a .android.js and a .ios.js, how about we have a single file ZulipAsyncStorage-test.js, and use Platform.OS conditionals there just like we normally do?

That would avoid some of the mess you're seeing in the first few commits in the branch with trying to adapt Flow to this situation. Fundamentally Flow wants to typecheck one coherent set of files, and not try to manage two conflicting slightly-different sets of them; if you want that, the answer is to run it twice.

It'd also eliminate some duplication, which actually seems to make up a substantial fraction of what's in the platform-specific files.

In general those are basically the same reasons we avoid the alternate-files thing in the rest of our code, and I think they generally apply to tests too.

For tests, it also looks to be what RN itself does -- here's in my RN tree:

$ git ls-files | grep -P 'test\.\w+\.js'
$

No test files at all that use the .android.js or .ios.js naming. Whereas:

$ rg -w 'OS ===' -- $(git ls-files | grep -e -test.js)
Libraries/Network/__tests__/XMLHttpRequest-test.js
21:  if (Platform.OS === 'ios') {

Libraries/StyleSheet/__tests__/normalizeColor-test.js
26:  if (OS === 'ios') {
64:  if (OS === 'android') {

Libraries/StyleSheet/__tests__/processColor-test.js
24:  OS === 'android'
96:    if (OS === 'ios') {
114:    if (OS === 'android') {

Libraries/StyleSheet/__tests__/processColorArray-test.js
24:  OS === 'android'
93:    if (OS === 'ios') {
121:    if (OS === 'android') {

there are at least a handful of tests that switch on platform. Adding context to the search, here's some of the ways they do it:

$ rg -wB2 -A6 'OS ===' -- $(git ls | grep -e -test.js)
…
20-function setRequestId(id) {
21:  if (Platform.OS === 'ios') {
22-    return;
23-  }
24-  requestId = id;
25-}
…
92-  describe('iOS', () => {
93:    if (OS === 'ios') {
94-      it('should convert array of iOS PlatformColor colors', () => {
95-        const colorFromArray = processColorArray([
96-          PlatformColorIOS('systemColorWhite'),
97-          PlatformColorIOS('systemColorBlack'),
98-        ]);
…

[gnprice]

From that Flow issue thread you linked to (in the comment above), I believe the story is that Flow will only look at one of these for any given module, namely the first one it finds. Does that agree with your understanding?

In particular, if there's both a foo.android.js and a foo.ios.js, it'll only check one of them.

Do you have a sense of what the pattern is on that? I'd guess that if we put .android.js first, as here, then Flow will find the .android.js before the .ios.js, and that'll be the one it runs with.

In any case whatever we believe the behavior is, it probably deserves a comment here.

[chrisbobbe]

Hmm, from looking closer just now, I don't think I've totally nailed down the pattern. Some observations, though:

I've temporarily tried splitting ZulipButton.js into ZulipButton.android.js and ZulipButton.ios.js.

I notice that the invalid assertion (123: string) gets caught in both files—as long as this bit under [ignore] is removed first:

-; We fork some components by platform
-.*/*[.]android.js

With that intact, the error in the .android.js file isn't reported.

So that makes sense, I guess?

But then, I added an android: boolean prop to ZulipButton.android.js, and an ios: boolean prop to ZulipButton.ios.js, to see what the complaints were at ZulipButton's callsites that obviously weren't passing either prop.

I got the error message from ZulipButton.android.js—property android is missing in props—when I tried with all these orderings:

module.file_ext=.json
module.file_ext=.js
module.file_ext=.android.js
module.file_ext=.ios.js

module.file_ext=.json
module.file_ext=.js
module.file_ext=.ios.js
module.file_ext=.android.js

module.file_ext=.json
module.file_ext=.android.js
module.file_ext=.ios.js
module.file_ext=.js

module.file_ext=.json
module.file_ext=.android.js
module.file_ext=.js
module.file_ext=.ios.js

module.file_ext=.json
module.file_ext=.ios.js
module.file_ext=.js
module.file_ext=.android.js

module.file_ext=.json
module.file_ext=.ios.js
module.file_ext=.android.js
module.file_ext=.js

Only when I commented out the .android.js line did I see the error message from ZulipButton.ios.js.

[chrisbobbe]

So this does point to generally not wanting to make .android.js and .ios.js files.

My reason for adding the module.file_ext=.android.js line was because react-native-paper reportedly requires it as part of its setup, and I thought this would be a good commit for the change to go in—but I didn't get to the point of testing that react-native-paper actually needs it, and we can do that at the time we install react-native-paper if we do.

For now I'll not add the module.file_ext=.android.js line. The .*/*[.]android.js cleanup seems OK though?

[chrisbobbe]

The .*/*[.]android.js cleanup seems OK though?

Hmm actually maybe better drop this, too, since I don't immediately need it (I'm folding the -test.android.js test files into the regular -test.js files as you've suggested). It's in the RN template app's Flow config. I'll probably thank myself when I'm doing a future RN upgrade.

[gnprice]

But then, I added an android: boolean prop to ZulipButton.android.js, and an ios: boolean prop to ZulipButton.ios.js, to see what the complaints were at ZulipButton's callsites that obviously weren't passing either prop.

Cool. From that data, I think a plausible hypothesis is:

• Flow will check each file that it finds at all. At this level, foo.android.js and foo.js are just two different files matching *.js; it doesn't care that one of the names has an extra . in it, and maybe doesn't even look at module.file_ext.
• But when checking some other file and importing a module foo, it consults module.file_ext to see what filenames could count: it wants to take the module name and append one of those extensions to get the filename.
• And if there are several extensions that lead to a valid file for the desired module name... well, apparently it doesn't care about the order the module.file_ext lines appear in the config. Maybe it just looks alphabetically? But then it'd be essential that both "android" and "ios" come before "js" in the alphabet... Or maybe it goes longest-first? That'd be a way of both ensuring .whatever.js comes before .js, and by happenstance putting .android.js before .js.

Docs don't seem to answer this question:
https://flow.org/en/docs/config/options/#toc-module-file-ext-string
but it's moot at least for now, so 🤷 .

[gnprice]

Nice to clean this up 🙂

[gnprice]

Hmm, I don't think this translation is quite right.

The thing is that chaining .then after .catch doesn't do what you want -- the catch returns a new Promise, one that if the .catch callback gets invoked, gets resolved with whatever that callback returned.

Quick demo:

$ node
> (async function () { throw new Error(); })().catch(e => {}).then(result => console.log(`got result: ${result}`))
Promise { <pending> }
> got result: undefined

I think you could get an accurate result by saving the storage.getAllKeys() promise as a local, and invoking both .catch and .then on that same promise.

... Oh hey, or alternatively: then takes two arguments, one for success and then one for failure, so you could make one .then call and pass both. That's pretty ugly, though, in that it separates the error handler way down away from the error source, after all the code for success. Good to know for cases when the success handler is much shorter, though.

In any case I think this is a nice demonstration of why working with .catch and .then is a pain to do correctly, and await is much better 😛 . Converting this code to await might be a bigger refactor, though.

[chrisbobbe]

Thanks for catching this! I could do an async IIFE with await and try/catch; what do you think?

[gnprice]

I see you've done that in the new revision. It looks like a nice clean translation!

-  storage.getAllKeys((err, allKeys) => {
+  (async () => {
+    let err
+    let allKeys
+    try {
+      allKeys = await storage.getAllKeys()
+    } catch (e) {
+      err = e
+    }

[chrisbobbe]

Thanks for the reviews! Revision pushed.

[gnprice]

Thanks for the revision! Looks good -- small comments below, then please merge at will.

[gnprice]

Should say let err = undefined;. This would be a real small nit except that plain let foo without initializer turns out to trigger a Flow bug; from our eslintrc:

  # We prefer `let foo = undefined;` over `let foo;`, not vice versa.
  #
  # Sadly there doesn't seem to be a rule to say what we want, but at least
  # turn off the one that says the opposite.
  #
  # This rule is dubious to begin with, because explicit is better than
  # implicit.  Then for us, the overriding reason we definitely don't
  # want it is that plain `let foo;` can trigger a Flow bug:
  #   https://github.com/facebook/flow/issues/8526
  # while `let foo = undefined;` makes Flow handle the logic smoothly.
  no-undef-init: off

[chrisbobbe]

Thanks! Actually, I think maybe err here and elsewhere in this commit should actually be initialized to null? If my comment above at #4709 (comment) points in the right direction for AsyncStorage.getAllKeys's implementation, I think null was being passed to the callback, not undefined.

[gnprice]

Hmm, and in fact this one should be initialized to null, not undefined! So I guess that illustrates the other reason to have an initializer, i.e. for explicitness 🙂

The reason is that (a) that's the direct translation of the old code, which would pass null to the callback, and (b) for this particular variable, the distinction matters because it gets passed to rehydrate and thence to JSON.parse, which throws an exception on undefined but just returns null for null. I think it all ends up working out the same because of the control flow in rehydrate, but would rather not have to depend on that detail.

[chrisbobbe]

Interesting, thanks for catching that!

[chrisbobbe]

Thanks for the review! I know you said to merge right away, but I've also changed the err initializers to use null, and I want to check that that change is correct. I think it is, but I'd like to double-check before merging.

[gnprice]

LGTM, thanks -- merged!

[chrisbobbe]

Thanks for the review!