From 96b796b51342fce9979c522c38cc753c0cc35c32 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 20 May 2021 21:55:56 -0400 Subject: [PATCH 01/14] MainTabsScreen [nfc]: Make it clear that `isPad` is iOS-only. Although it doesn't give a reason why, the `Platform` doc [1] is clear that `isPad` is iOS-only; we can expect it to be missing on Android. We'd love to know whether an Android user is on a tablet, but it seems `Platform` doesn't yet give us a way to do that. Until it does, or until we use something else that does, make it really clear that we're not getting accurate info on Android. [1] https://reactnative.dev/docs/platform#ispad-ios --- src/main/MainTabsScreen.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/main/MainTabsScreen.js b/src/main/MainTabsScreen.js index 10378d657dd..acf6a546ad8 100644 --- a/src/main/MainTabsScreen.js +++ b/src/main/MainTabsScreen.js @@ -52,7 +52,12 @@ export default function MainTabsScreen(props: Props) { Date: Mon, 26 Apr 2021 15:13:43 -0400 Subject: [PATCH 02/14] ZulipAsyncStorage tests: Add tests for `setItem`. With a new mock for `TextCompressionModule`; we do that globally in case it's useful elsewhere. That module should have its own set of unit tests, if we plan to keep it; it looks like it doesn't yet. --- jest/jestSetup.js | 11 +++ src/boot/__tests__/ZulipAsyncStorage-test.js | 80 ++++++++++++++++++++ 2 files changed, 91 insertions(+) create mode 100644 src/boot/__tests__/ZulipAsyncStorage-test.js diff --git a/jest/jestSetup.js b/jest/jestSetup.js index 4ea7b01af43..dd071e5819a 100644 --- a/jest/jestSetup.js +++ b/jest/jestSetup.js @@ -45,6 +45,17 @@ jest.mock('react-native', () => { resourceURL: 'file:///private/var/containers/Bundle/Application/4DCD4D2B-F745-4C70-AD74-8E5F690CF593/ZulipMobile.app/', }; + } else if (ReactNative.Platform.OS === 'android') { + const header = 'z|mock|'; + ReactNative.NativeModules.TextCompressionModule = { + header, + compress: jest.fn( + async (input: string) => `${header}${Buffer.from(input).toString('base64')}`, + ), + decompress: jest.fn(async (input: string) => + Buffer.from(input.replace(header, ''), 'base64').toString('utf8'), + ), + }; } return ReactNative; diff --git a/src/boot/__tests__/ZulipAsyncStorage-test.js b/src/boot/__tests__/ZulipAsyncStorage-test.js new file mode 100644 index 00000000000..04b1f81fc0c --- /dev/null +++ b/src/boot/__tests__/ZulipAsyncStorage-test.js @@ -0,0 +1,80 @@ +/* @flow strict-local */ +import { Platform, NativeModules } from 'react-native'; +import AsyncStorage from '@react-native-community/async-storage'; +import ZulipAsyncStorage from '../ZulipAsyncStorage'; + +describe('setItem', () => { + const key = 'foo!'; + const value = '123!'; + const callback = jest.fn(); + beforeEach(() => callback.mockClear()); + + // For checking that AsyncStorage.setItem is called in ways we expect. + const asyncStorageSetItemSpy = jest.spyOn(AsyncStorage, 'setItem'); + beforeEach(() => asyncStorageSetItemSpy.mockClear()); + + const run = async () => ZulipAsyncStorage.setItem(key, value, callback); + + describe('success', () => { + // AsyncStorage provides its own mock for `.setItem`, which gives + // success every time. So, no need to mock that behavior + // ourselves. + + test('resolves correctly', async () => { + await expect(run()).resolves.toBe(null); + }); + + test('callback called correctly', async () => { + await run(); + expect(callback).toHaveBeenCalledTimes(1); + expect(callback).toHaveBeenCalledWith(null); + }); + + test('AsyncStorage.setItem called correctly', async () => { + await run(); + expect(asyncStorageSetItemSpy).toHaveBeenCalledTimes(1); + expect(asyncStorageSetItemSpy).toHaveBeenCalledWith( + key, + Platform.OS === 'ios' ? value : await NativeModules.TextCompressionModule.compress(value), + callback, + ); + }); + }); + + describe('failure', () => { + // AsyncStorage provides its own mock for `.setItem`, but it's + // not set up to simulate failure. So, mock that behavior + // ourselves, and reset to the global mock when we're done. + const globalMock = AsyncStorage.setItem; + beforeEach(() => { + // $FlowFixMe[cannot-write] Make Flow understand about mocking. + AsyncStorage.setItem = jest.fn( + async (k: string, v: string, cb?: ?(e: ?Error) => void): Promise => { + const error = new Error(); + if (cb) { + cb(error); + } + throw error; + }, + ); + }); + afterAll(() => { + // $FlowFixMe[cannot-write] Make Flow understand about mocking. + AsyncStorage.setItem = globalMock; + }); + + test('rejects correctly', async () => { + await expect(run()).rejects.toThrow(Error); + }); + + test('callback called correctly', async () => { + try { + await run(); + } catch (e) { + // "rejects correctly" covers this + } + expect(callback).toHaveBeenCalledTimes(1); + expect(callback).toHaveBeenCalledWith(expect.any(Error)); + }); + }); +}); From f9bbdd00474ab1d5e39a6c7cd922c9e860754e13 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 26 Apr 2021 16:06:34 -0400 Subject: [PATCH 03/14] TextCompressionModule [nfc]: Add TODO for unit tests. --- .../src/main/java/com/zulipmobile/TextCompressionModule.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/android/app/src/main/java/com/zulipmobile/TextCompressionModule.java b/android/app/src/main/java/com/zulipmobile/TextCompressionModule.java index e9c809682ab..876be5448b8 100644 --- a/android/app/src/main/java/com/zulipmobile/TextCompressionModule.java +++ b/android/app/src/main/java/com/zulipmobile/TextCompressionModule.java @@ -14,6 +14,8 @@ import java.util.zip.Deflater; import java.util.zip.Inflater; +// TODO: Write unit tests; see +// https://github.com/zulip/zulip-mobile/blob/master/docs/howto/testing.md#unit-tests-android. class TextCompressionModule extends ReactContextBaseJavaModule { public TextCompressionModule(ReactApplicationContext reactContext) { super(reactContext); From b35aeb9c9a7d3b506379a358018006a3bd77229a Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Apr 2021 16:17:57 -0400 Subject: [PATCH 04/14] ZulipAsyncStorage types: Better align .setItem/.getItem with AsyncStorage. Now, the type for the `callback` param matches what's in the libdef for `AsyncStorage`. I think the question mark in the `?(...) => Foo` syntax isn't what we'd normally choose, but `ZulipAsyncStorage` is a wrapper around `AsyncStorage` so it might as well match this detail. Also the callback in `getItem` wasn't being considered optional at all, and now it is, which it looks like our implementation expects. --- src/boot/ZulipAsyncStorage.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/boot/ZulipAsyncStorage.js b/src/boot/ZulipAsyncStorage.js index bfabe1e3440..589c418bf31 100644 --- a/src/boot/ZulipAsyncStorage.js +++ b/src/boot/ZulipAsyncStorage.js @@ -4,7 +4,7 @@ import { NativeModules } from 'react-native'; import * as logging from '../utils/logging'; export default class ZulipAsyncStorage { - static async getItem(key: string, callback: (error: ?Error, result: ?string) => void) { + static async getItem(key: string, callback?: ?(error: ?Error, result: ?string) => void) { let result = await AsyncStorage.getItem(key); // It's possible that getItem() is called on uncompressed state, for // example when a user updates their app from a version without @@ -48,7 +48,7 @@ export default class ZulipAsyncStorage { return result; } - static async setItem(key: string, value: string, callback: ?(error: ?Error) => void) { + static async setItem(key: string, value: string, callback?: ?(error: ?Error) => void) { if (!NativeModules.TextCompressionModule) { return AsyncStorage.setItem(key, value, callback); } From 21f12c8075cc49254ed15d1bb12bba95f124abfa Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Apr 2021 16:53:28 -0400 Subject: [PATCH 05/14] ZulipAsyncStorage: Pass null for error, not undefined, on .getItem success. `null` gets passed on `.setItem`'s success, so we might as well be consistent and do the same thing with `.getItem`. I'm starting to think that `.getItem` and `.setItem`'s callback-based interfaces can be trimmed away, and callers can exclusively use the returned Promise. --- src/boot/ZulipAsyncStorage.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/boot/ZulipAsyncStorage.js b/src/boot/ZulipAsyncStorage.js index 589c418bf31..54dee2f6dcf 100644 --- a/src/boot/ZulipAsyncStorage.js +++ b/src/boot/ZulipAsyncStorage.js @@ -43,7 +43,7 @@ export default class ZulipAsyncStorage { } } if (callback) { - callback(undefined, result); + callback(null, result); } return result; } From d945e4576b5e87c3d2bc221b003b233083b9d412 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Apr 2021 16:56:28 -0400 Subject: [PATCH 06/14] ZulipAsyncStorage: Also call callback on `AsyncStorage.getItem` failure. A bug like this is another reason I'm continuing to think we should just not bother to maintain a callback-based interface, here and in `.setItem`, and just maintain the Promise-based interface. Before we vendored redux-persist, which is the home of these methods' only callers, this would have been trickier. But now it's easy to make those callers adapt to the Promise-based interface. --- src/boot/ZulipAsyncStorage.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/boot/ZulipAsyncStorage.js b/src/boot/ZulipAsyncStorage.js index 54dee2f6dcf..2b3713c3f00 100644 --- a/src/boot/ZulipAsyncStorage.js +++ b/src/boot/ZulipAsyncStorage.js @@ -5,7 +5,16 @@ import * as logging from '../utils/logging'; export default class ZulipAsyncStorage { static async getItem(key: string, callback?: ?(error: ?Error, result: ?string) => void) { - let result = await AsyncStorage.getItem(key); + let result; + try { + result = await AsyncStorage.getItem(key); + } catch (err) { + if (callback) { + callback(err, null); + } + throw err; + } + // It's possible that getItem() is called on uncompressed state, for // example when a user updates their app from a version without // compression to a version with compression. So we need to detect that. From e91d9a8b4422b2fa8e5be9363dfe53f78aa62418 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Apr 2021 16:14:56 -0400 Subject: [PATCH 07/14] ZulipAsyncStorage tests: Add tests for `getItem`. --- src/boot/__tests__/ZulipAsyncStorage-test.js | 106 +++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/src/boot/__tests__/ZulipAsyncStorage-test.js b/src/boot/__tests__/ZulipAsyncStorage-test.js index 04b1f81fc0c..8a09e956a80 100644 --- a/src/boot/__tests__/ZulipAsyncStorage-test.js +++ b/src/boot/__tests__/ZulipAsyncStorage-test.js @@ -2,6 +2,7 @@ import { Platform, NativeModules } from 'react-native'; import AsyncStorage from '@react-native-community/async-storage'; import ZulipAsyncStorage from '../ZulipAsyncStorage'; +import * as logging from '../../utils/logging'; describe('setItem', () => { const key = 'foo!'; @@ -78,3 +79,108 @@ describe('setItem', () => { }); }); }); + +describe('getItem', () => { + const key = 'foo!'; + const value = '123!'; + const callback = jest.fn(); + beforeEach(() => callback.mockClear()); + + // For checking that AsyncStorage.getItem is called in ways we expect. + const asyncStorageGetItemSpy = jest.spyOn(AsyncStorage, 'getItem'); + beforeEach(() => asyncStorageGetItemSpy.mockClear()); + + beforeAll(async () => { + // `AsyncStorage` mocks storage by writing to a variable instead + // of to the disk. Put something there for our + // `ZulipAsyncStorage.getItem` to retrieve. + await AsyncStorage.setItem( + key, + Platform.OS === 'ios' ? value : await NativeModules.TextCompressionModule.compress(value), + ); + + // suppress `logging.error` output + // $FlowFixMe - teach Flow about mocks + logging.error.mockReturnValue(); + }); + + const run = async () => ZulipAsyncStorage.getItem(key, callback); + + describe('success', () => { + // AsyncStorage provides its own mock for `.getItem`, which gives + // success every time. So, no need to mock that behavior + // ourselves. + + test('calls AsyncStorage.getItem as we expect it to', async () => { + await run(); + expect(asyncStorageGetItemSpy).toHaveBeenCalledTimes(1); + expect(asyncStorageGetItemSpy).toHaveBeenCalledWith(key); + }); + + test('resolves correctly', async () => { + await expect(run()).resolves.toBe(value); + }); + + test('callback called correctly', async () => { + await run(); + expect(callback).toHaveBeenCalledTimes(1); + expect(callback).toHaveBeenCalledWith(null, value); + }); + }); + + describe('failure', () => { + // AsyncStorage provides its own mock for `.getItem`, but it's + // not set up to simulate failure. So, mock that behavior + // ourselves. + const globalMock = AsyncStorage.getItem; + beforeEach(() => { + // $FlowFixMe[cannot-write] Make Flow understand about mocking. + AsyncStorage.getItem = jest.fn( + async (k: string, cb?: ?(e: ?Error, r: string | null) => void): Promise => { + const error = new Error(); + if (cb) { + cb(error, null); + } + throw error; + }, + ); + }); + afterAll(() => { + // $FlowFixMe[cannot-write] Make Flow understand about mocking. + AsyncStorage.getItem = globalMock; + }); + + test('rejects correctly', async () => { + await expect(run()).rejects.toThrow(Error); + }); + + test('callback called correctly', async () => { + try { + await run(); + } catch (e) { + // "rejects correctly" covers this + } + expect(callback).toHaveBeenCalledTimes(1); + expect(callback).toHaveBeenCalledWith(expect.any(Error), null); + }); + }); + + if (Platform.OS === 'android') { + describe('android', () => { + test('panics when value is in unknown compression format', async () => { + const unknownHeader = 'z|mock-unknown-format|'; + + await AsyncStorage.setItem( + `${key}-unknown`, + `${unknownHeader}${Buffer.from('123!').toString('hex')}`, + ); + + await expect(ZulipAsyncStorage.getItem(`${key}-unknown`, callback)).rejects.toThrow( + `No decompression module found for format ${unknownHeader}`, + ); + expect(callback).toHaveBeenCalledTimes(1); + expect(callback).toHaveBeenCalledWith(expect.any(Error), null); + }); + }); + } +}); From f46368f9d0a8dfb07b252d1d6fc85ef6fc328bfc Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Apr 2021 21:02:46 -0400 Subject: [PATCH 08/14] ZulipAsyncStorage tests: Add test for `getItem` / `setItem` together. --- src/boot/__tests__/ZulipAsyncStorage-test.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/boot/__tests__/ZulipAsyncStorage-test.js b/src/boot/__tests__/ZulipAsyncStorage-test.js index 8a09e956a80..10424e75721 100644 --- a/src/boot/__tests__/ZulipAsyncStorage-test.js +++ b/src/boot/__tests__/ZulipAsyncStorage-test.js @@ -3,6 +3,7 @@ import { Platform, NativeModules } from 'react-native'; import AsyncStorage from '@react-native-community/async-storage'; import ZulipAsyncStorage from '../ZulipAsyncStorage'; import * as logging from '../../utils/logging'; +import * as eg from '../../__tests__/lib/exampleData'; describe('setItem', () => { const key = 'foo!'; @@ -184,3 +185,15 @@ describe('getItem', () => { }); } }); + +describe('setItem/getItem together', () => { + test('round-tripping works', async () => { + const key = eg.randString(); + const value = eg.randString(); + + // AsyncStorage provides its own mocks for `.getItem` and + // `.setItem`; it writes to a variable instead of storage. + await ZulipAsyncStorage.setItem(key, value); + expect(await ZulipAsyncStorage.getItem(key)).toEqual(value); + }); +}); From 317e9ead3d13eed7ff865300e1450856c0577c38 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Apr 2021 18:29:58 -0400 Subject: [PATCH 09/14] redux-persist [nfc]: Remove some inactive code paths we'll never use. We don't and can't use `window.localStorage` [1], `window.sessionStorage` [2], or `localforage` [3]; those are (or they use) web APIs. So, trim all this stuff that's related to those out. I think I got it all, and no more; anyway, the app still runs, and storage seems to work fine. [1] https://developer.mozilla.org/en-US/docs/Web/API/Window/localStorage [2] https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage [3] https://github.com/localForage/localForage --- src/third/redux-persist/createPersistor.js | 7 +- src/third/redux-persist/defaultStorages.js | 4 - .../defaults/asyncLocalStorage.js | 113 ------------------ src/third/redux-persist/getStoredState.js | 6 +- src/third/redux-persist/index.js | 15 +-- 5 files changed, 3 insertions(+), 142 deletions(-) delete mode 100644 src/third/redux-persist/defaultStorages.js delete mode 100644 src/third/redux-persist/defaults/asyncLocalStorage.js diff --git a/src/third/redux-persist/createPersistor.js b/src/third/redux-persist/createPersistor.js index feb24a434f8..73a08d11e44 100644 --- a/src/third/redux-persist/createPersistor.js +++ b/src/third/redux-persist/createPersistor.js @@ -1,5 +1,4 @@ import { KEY_PREFIX, REHYDRATE } from './constants' -import createAsyncLocalStorage from './defaults/asyncLocalStorage' import purgeStoredState from './purgeStoredState' import stringify from 'json-stringify-safe' @@ -34,11 +33,7 @@ export default function createPersistor (store, config) { const stateGetter = config._stateGetter || defaultStateGetter const stateSetter = config._stateSetter || defaultStateSetter - // storage with keys -> getAllKeys for localForage support - let storage = config.storage || createAsyncLocalStorage('local') - if (storage.keys && !storage.getAllKeys) { - storage.getAllKeys = storage.keys - } + const storage = config.storage; // initialize stateful values let lastState = stateInit diff --git a/src/third/redux-persist/defaultStorages.js b/src/third/redux-persist/defaultStorages.js deleted file mode 100644 index cec7f9442b5..00000000000 --- a/src/third/redux-persist/defaultStorages.js +++ /dev/null @@ -1,4 +0,0 @@ -import createAsyncLocalStorage from './defaults/asyncLocalStorage' - -export const asyncLocalStorage = createAsyncLocalStorage('local') -export const asyncSessionStorage = createAsyncLocalStorage('session') diff --git a/src/third/redux-persist/defaults/asyncLocalStorage.js b/src/third/redux-persist/defaults/asyncLocalStorage.js deleted file mode 100644 index e3209d1f5e7..00000000000 --- a/src/third/redux-persist/defaults/asyncLocalStorage.js +++ /dev/null @@ -1,113 +0,0 @@ -import setImmediate from '../utils/setImmediate' - -let noStorage = () => { /* noop */ return null } -if (process.env.NODE_ENV !== 'production') { - noStorage = () => { - console.error('redux-persist asyncLocalStorage requires a global localStorage object. Either use a different storage backend or if this is a universal redux application you probably should conditionally persist like so: https://gist.github.com/rt2zz/ac9eb396793f95ff3c3b') - return null - } -} - -function _hasStorage (storageType) { - if (typeof window !== 'object' || !(storageType in window)) { - return false - } - - try { - let storage = window[storageType] - const testKey = `redux-persist ${storageType} test` - storage.setItem(testKey, 'test') - storage.getItem(testKey) - storage.removeItem(testKey) - } catch (e) { - if (process.env.NODE_ENV !== 'production') console.warn(`redux-persist ${storageType} test failed, persistence will be disabled.`) - return false - } - return true -} - -function hasLocalStorage () { - return _hasStorage('localStorage') -} - -function hasSessionStorage () { - return _hasStorage('sessionStorage') -} - -function getStorage (type) { - if (type === 'local') { - return hasLocalStorage() - ? window.localStorage - : { getItem: noStorage, setItem: noStorage, removeItem: noStorage, getAllKeys: noStorage } - } - if (type === 'session') { - return hasSessionStorage() - ? window.sessionStorage - : { getItem: noStorage, setItem: noStorage, removeItem: noStorage, getAllKeys: noStorage } - } -} - -export default function (type, config) { - let storage = getStorage(type) - return { - getAllKeys: function (cb) { - return new Promise((resolve, reject) => { - try { - var keys = [] - for (var i = 0; i < storage.length; i++) { - keys.push(storage.key(i)) - } - setImmediate(() => { - cb && cb(null, keys) - resolve(keys) - }) - } catch (e) { - cb && cb(e) - reject(e) - } - }) - }, - getItem (key, cb) { - return new Promise((resolve, reject) => { - try { - var s = storage.getItem(key) - setImmediate(() => { - cb && cb(null, s) - resolve(s) - }) - } catch (e) { - cb && cb(e) - reject(e) - } - }) - }, - setItem (key, string, cb) { - return new Promise((resolve, reject) => { - try { - storage.setItem(key, string) - setImmediate(() => { - cb && cb(null) - resolve() - }) - } catch (e) { - cb && cb(e) - reject(e) - } - }) - }, - removeItem (key, cb) { - return new Promise((resolve, reject) => { - try { - storage.removeItem(key) - setImmediate(() => { - cb && cb(null) - resolve() - }) - } catch (e) { - cb && cb(e) - reject(e) - } - }) - } - } -} diff --git a/src/third/redux-persist/getStoredState.js b/src/third/redux-persist/getStoredState.js index c709c96084a..e579abdd6fa 100644 --- a/src/third/redux-persist/getStoredState.js +++ b/src/third/redux-persist/getStoredState.js @@ -1,8 +1,7 @@ import { KEY_PREFIX } from './constants' -import createAsyncLocalStorage from './defaults/asyncLocalStorage' export default function getStoredState (config, onComplete) { - let storage = config.storage || createAsyncLocalStorage('local') + let storage = config.storage let deserializer; if (config.deserialize === false) { deserializer = (data) => data @@ -16,9 +15,6 @@ export default function getStoredState (config, onComplete) { const transforms = config.transforms || [] const keyPrefix = config.keyPrefix !== undefined ? config.keyPrefix : KEY_PREFIX - // fallback getAllKeys to `keys` if present (LocalForage compatability) - if (storage.keys && !storage.getAllKeys) storage = {...storage, getAllKeys: storage.keys} - let restoredState = {} let completionCount = 0 diff --git a/src/third/redux-persist/index.js b/src/third/redux-persist/index.js index 4b4e95d08ca..75269fb6bc0 100644 --- a/src/third/redux-persist/index.js +++ b/src/third/redux-persist/index.js @@ -5,17 +5,4 @@ import getStoredState from './getStoredState' import persistStore from './persistStore' import purgeStoredState from './purgeStoredState' -// @TODO remove in v5 -const deprecated = (cb, cb2, cb3) => { - console.error('redux-persist: this method of importing storages has been removed. instead use `import { asyncLocalStorage } from "redux-persist/storages"`') - if (typeof cb === 'function') cb() - if (typeof cb2 === 'function') cb2() - if (typeof cb3 === 'function') cb3() -} -const deprecatedStorage = { getAllKeys: deprecated, getItem: deprecated, setItem: deprecated, removeItem: deprecated } -const storages = { - asyncLocalStorage: deprecatedStorage, - asyncSessionStorage: deprecatedStorage -} - -export { autoRehydrate, createPersistor, createTransform, getStoredState, persistStore, purgeStoredState, storages } +export { autoRehydrate, createPersistor, createTransform, getStoredState, persistStore, purgeStoredState } From 3737f61a96a8946a7d85edf987cc325e194e130a Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Apr 2021 20:31:35 -0400 Subject: [PATCH 10/14] redux-persist: Make early `return` explicit. It looks like the rest of this callback is only meant to run if there hasn't been an error. In practice, I think most of it wasn't running anyway, since there would have been an error like "TypeError: Cannot read property 'filter' of undefined" on the `allKeys.filter(...)` just below. Good to be explicit, though. --- src/third/redux-persist/getStoredState.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/third/redux-persist/getStoredState.js b/src/third/redux-persist/getStoredState.js index e579abdd6fa..17f805a6d73 100644 --- a/src/third/redux-persist/getStoredState.js +++ b/src/third/redux-persist/getStoredState.js @@ -22,6 +22,7 @@ export default function getStoredState (config, onComplete) { if (err) { if (process.env.NODE_ENV !== 'production') console.warn('redux-persist/getStoredState: Error in storage.getAllKeys') complete(err) + return } let persistKeys = allKeys.filter((key) => key.indexOf(keyPrefix) === 0).map((key) => key.slice(keyPrefix.length)) From a81847f7fd6597aea8889f4a6ab51f2a1e599b24 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Apr 2021 19:57:48 -0400 Subject: [PATCH 11/14] redux-persist [nfc]: Stop using callbacks with `ZulipAsyncStorage.foo`. Instead, use these functions' returned Promises. This will let us drop ZulipAsyncStorage's callback-based interface, which will simplify its code. Done carefully to preserve the control flow. --- src/third/redux-persist/createPersistor.js | 2 +- src/third/redux-persist/getStoredState.js | 25 ++++++++++++++++----- src/third/redux-persist/purgeStoredState.js | 20 +++++++++++++---- 3 files changed, 37 insertions(+), 10 deletions(-) diff --git a/src/third/redux-persist/createPersistor.js b/src/third/redux-persist/createPersistor.js index 73a08d11e44..09216e86b57 100644 --- a/src/third/redux-persist/createPersistor.js +++ b/src/third/redux-persist/createPersistor.js @@ -67,7 +67,7 @@ export default function createPersistor (store, config) { let key = storesToProcess.shift() let storageKey = createStorageKey(key) let endState = transforms.reduce((subState, transformer) => transformer.in(subState, key), stateGetter(store.getState(), key)) - if (typeof endState !== 'undefined') storage.setItem(storageKey, serializer(endState), warnIfSetError(key)) + if (typeof endState !== 'undefined') storage.setItem(storageKey, serializer(endState)).catch(warnIfSetError(key)) }, debounce) } diff --git a/src/third/redux-persist/getStoredState.js b/src/third/redux-persist/getStoredState.js index 17f805a6d73..2a06bdc568c 100644 --- a/src/third/redux-persist/getStoredState.js +++ b/src/third/redux-persist/getStoredState.js @@ -16,9 +16,17 @@ export default function getStoredState (config, onComplete) { const keyPrefix = config.keyPrefix !== undefined ? config.keyPrefix : KEY_PREFIX let restoredState = {} - let completionCount = 0 + let completionCount = 0; + + (async () => { + let err = null; + let allKeys = undefined; + try { + allKeys = await storage.getAllKeys() + } catch (e) { + err = e + } - storage.getAllKeys((err, allKeys) => { if (err) { if (process.env.NODE_ENV !== 'production') console.warn('redux-persist/getStoredState: Error in storage.getAllKeys') complete(err) @@ -31,14 +39,21 @@ export default function getStoredState (config, onComplete) { let restoreCount = keysToRestore.length if (restoreCount === 0) complete(null, restoredState) keysToRestore.forEach((key) => { - storage.getItem(createStorageKey(key), (err, serialized) => { + (async () => { + let err = null; + let serialized = null; + try { + serialized = await storage.getItem(createStorageKey(key)) + } catch (e) { + err = e + } if (err && process.env.NODE_ENV !== 'production') console.warn('redux-persist/getStoredState: Error restoring data for key:', key, err) else restoredState[key] = rehydrate(key, serialized) completionCount += 1 if (completionCount === restoreCount) complete(null, restoredState) - }) + })() }) - }) + })(); function rehydrate (key, serialized) { let state = null diff --git a/src/third/redux-persist/purgeStoredState.js b/src/third/redux-persist/purgeStoredState.js index 04c26511557..368ac28d23f 100644 --- a/src/third/redux-persist/purgeStoredState.js +++ b/src/third/redux-persist/purgeStoredState.js @@ -10,18 +10,30 @@ export default function purgeStoredState (config, keys) { if (typeof keys === 'undefined') { // if keys is not defined, purge all keys return new Promise((resolve, reject) => { - storage.getAllKeys((err, allKeys) => { + (async () => { + let err = null; + let allKeys = undefined; + try { + allKeys = await storage.getAllKeys() + } catch (e) { + err = e + } if (err) { if (process.env.NODE_ENV !== 'production') console.warn('redux-persist: error during purgeStoredState in storage.getAllKeys') reject(err) } else { resolve(purgeStoredState(config, allKeys.filter((key) => key.indexOf(keyPrefix) === 0).map((key) => key.slice(keyPrefix.length)))) } - }) + })() }) } else { // otherwise purge specified keys - return Promise.all(keys.map((key) => { - return storage.removeItem(`${keyPrefix}${key}`, warnIfRemoveError(key)) + return Promise.all(keys.map(async (key) => { + try { + return await storage.removeItem(`${keyPrefix}${key}`) + } catch (err) { + warnIfRemoveError(key)(err) + throw err + } })) } } From 9fff76d9ee53e43f023db7b4d3092c65d77fda15 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Apr 2021 20:09:28 -0400 Subject: [PATCH 12/14] ZulipAsyncStorage [nfc]: Drop support for passing callbacks. This simplifies the code quite a bit. --- src/boot/ZulipAsyncStorage.js | 39 ++++------- src/boot/__tests__/ZulipAsyncStorage-test.js | 69 +++----------------- 2 files changed, 21 insertions(+), 87 deletions(-) diff --git a/src/boot/ZulipAsyncStorage.js b/src/boot/ZulipAsyncStorage.js index 2b3713c3f00..fb92672eac1 100644 --- a/src/boot/ZulipAsyncStorage.js +++ b/src/boot/ZulipAsyncStorage.js @@ -4,16 +4,8 @@ import { NativeModules } from 'react-native'; import * as logging from '../utils/logging'; export default class ZulipAsyncStorage { - static async getItem(key: string, callback?: ?(error: ?Error, result: ?string) => void) { - let result; - try { - result = await AsyncStorage.getItem(key); - } catch (err) { - if (callback) { - callback(err, null); - } - throw err; - } + static async getItem(key: string) { + const item = await AsyncStorage.getItem(key); // It's possible that getItem() is called on uncompressed state, for // example when a user updates their app from a version without @@ -31,13 +23,14 @@ export default class ZulipAsyncStorage { // E.g., `zlib base64` means DATA is a base64 encoding of a zlib // encoding of the underlying data. We call the "z|TRANSFORMS|" part // the "header" of the string. - if (result !== null && result.startsWith('z')) { - const header = result.substring(0, result.indexOf('|', result.indexOf('|') + 1) + 1); + if (item !== null && item.startsWith('z')) { + // In this block, `item` is compressed state. + const header = item.substring(0, item.indexOf('|', item.indexOf('|') + 1) + 1); if ( NativeModules.TextCompressionModule && header === NativeModules.TextCompressionModule.header ) { - result = await NativeModules.TextCompressionModule.decompress(result); + return NativeModules.TextCompressionModule.decompress(item); } else { // Panic! If we are confronted with an unknown format, there is // nothing we can do to save the situation. Log an error and ignore @@ -45,27 +38,19 @@ export default class ZulipAsyncStorage { // their version of the app. const err = new Error(`No decompression module found for format ${header}`); logging.error(err); - if (callback) { - callback(err, null); - } throw err; } } - if (callback) { - callback(null, result); - } - return result; + + // Uncompressed state + return item; } - static async setItem(key: string, value: string, callback?: ?(error: ?Error) => void) { + static async setItem(key: string, value: string) { if (!NativeModules.TextCompressionModule) { - return AsyncStorage.setItem(key, value, callback); + return AsyncStorage.setItem(key, value); } - return AsyncStorage.setItem( - key, - await NativeModules.TextCompressionModule.compress(value), - callback, - ); + return AsyncStorage.setItem(key, await NativeModules.TextCompressionModule.compress(value)); } static removeItem = AsyncStorage.removeItem; diff --git a/src/boot/__tests__/ZulipAsyncStorage-test.js b/src/boot/__tests__/ZulipAsyncStorage-test.js index 10424e75721..3b8684474a6 100644 --- a/src/boot/__tests__/ZulipAsyncStorage-test.js +++ b/src/boot/__tests__/ZulipAsyncStorage-test.js @@ -8,14 +8,12 @@ import * as eg from '../../__tests__/lib/exampleData'; describe('setItem', () => { const key = 'foo!'; const value = '123!'; - const callback = jest.fn(); - beforeEach(() => callback.mockClear()); // For checking that AsyncStorage.setItem is called in ways we expect. const asyncStorageSetItemSpy = jest.spyOn(AsyncStorage, 'setItem'); beforeEach(() => asyncStorageSetItemSpy.mockClear()); - const run = async () => ZulipAsyncStorage.setItem(key, value, callback); + const run = async () => ZulipAsyncStorage.setItem(key, value); describe('success', () => { // AsyncStorage provides its own mock for `.setItem`, which gives @@ -26,19 +24,12 @@ describe('setItem', () => { await expect(run()).resolves.toBe(null); }); - test('callback called correctly', async () => { - await run(); - expect(callback).toHaveBeenCalledTimes(1); - expect(callback).toHaveBeenCalledWith(null); - }); - test('AsyncStorage.setItem called correctly', async () => { await run(); expect(asyncStorageSetItemSpy).toHaveBeenCalledTimes(1); expect(asyncStorageSetItemSpy).toHaveBeenCalledWith( key, Platform.OS === 'ios' ? value : await NativeModules.TextCompressionModule.compress(value), - callback, ); }); }); @@ -50,15 +41,9 @@ describe('setItem', () => { const globalMock = AsyncStorage.setItem; beforeEach(() => { // $FlowFixMe[cannot-write] Make Flow understand about mocking. - AsyncStorage.setItem = jest.fn( - async (k: string, v: string, cb?: ?(e: ?Error) => void): Promise => { - const error = new Error(); - if (cb) { - cb(error); - } - throw error; - }, - ); + AsyncStorage.setItem = jest.fn(async (k: string, v: string): Promise => { + throw new Error(); + }); }); afterAll(() => { // $FlowFixMe[cannot-write] Make Flow understand about mocking. @@ -68,24 +53,12 @@ describe('setItem', () => { test('rejects correctly', async () => { await expect(run()).rejects.toThrow(Error); }); - - test('callback called correctly', async () => { - try { - await run(); - } catch (e) { - // "rejects correctly" covers this - } - expect(callback).toHaveBeenCalledTimes(1); - expect(callback).toHaveBeenCalledWith(expect.any(Error)); - }); }); }); describe('getItem', () => { const key = 'foo!'; const value = '123!'; - const callback = jest.fn(); - beforeEach(() => callback.mockClear()); // For checking that AsyncStorage.getItem is called in ways we expect. const asyncStorageGetItemSpy = jest.spyOn(AsyncStorage, 'getItem'); @@ -105,7 +78,7 @@ describe('getItem', () => { logging.error.mockReturnValue(); }); - const run = async () => ZulipAsyncStorage.getItem(key, callback); + const run = async () => ZulipAsyncStorage.getItem(key); describe('success', () => { // AsyncStorage provides its own mock for `.getItem`, which gives @@ -121,12 +94,6 @@ describe('getItem', () => { test('resolves correctly', async () => { await expect(run()).resolves.toBe(value); }); - - test('callback called correctly', async () => { - await run(); - expect(callback).toHaveBeenCalledTimes(1); - expect(callback).toHaveBeenCalledWith(null, value); - }); }); describe('failure', () => { @@ -136,15 +103,9 @@ describe('getItem', () => { const globalMock = AsyncStorage.getItem; beforeEach(() => { // $FlowFixMe[cannot-write] Make Flow understand about mocking. - AsyncStorage.getItem = jest.fn( - async (k: string, cb?: ?(e: ?Error, r: string | null) => void): Promise => { - const error = new Error(); - if (cb) { - cb(error, null); - } - throw error; - }, - ); + AsyncStorage.getItem = jest.fn(async (k: string): Promise => { + throw new Error(); + }); }); afterAll(() => { // $FlowFixMe[cannot-write] Make Flow understand about mocking. @@ -154,16 +115,6 @@ describe('getItem', () => { test('rejects correctly', async () => { await expect(run()).rejects.toThrow(Error); }); - - test('callback called correctly', async () => { - try { - await run(); - } catch (e) { - // "rejects correctly" covers this - } - expect(callback).toHaveBeenCalledTimes(1); - expect(callback).toHaveBeenCalledWith(expect.any(Error), null); - }); }); if (Platform.OS === 'android') { @@ -176,11 +127,9 @@ describe('getItem', () => { `${unknownHeader}${Buffer.from('123!').toString('hex')}`, ); - await expect(ZulipAsyncStorage.getItem(`${key}-unknown`, callback)).rejects.toThrow( + await expect(ZulipAsyncStorage.getItem(`${key}-unknown`)).rejects.toThrow( `No decompression module found for format ${unknownHeader}`, ); - expect(callback).toHaveBeenCalledTimes(1); - expect(callback).toHaveBeenCalledWith(expect.any(Error), null); }); }); } From cd29cb9c97302e9dabd248f8ee8f62844b49c565 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 29 Apr 2021 20:04:42 -0400 Subject: [PATCH 13/14] reduxTypes: Mark `GlobalState`'s properties as read-only. We already treat them as read-only everywhere except for one case, touched in this commit, where we've had to suppress several different Flow errors [0]. So, mark them as read-only with `$ReadOnly` [1]. Treating `GlobalState`'s properties as read-only (except for that one exceptional case) is likely to be very stable: we treat them as read-only because the Redux state is a tree of objects that are all treated as immutable; see the Redux docs [2] and our architecture doc [3]. [0] See Greg's comments on this at https://github.com/zulip/zulip-mobile/pull/4709#discussion_r638475577 [1] https://flow.org/en/docs/types/utilities/#toc-readonly [2] http://redux.js.org [3] ./docs/architecture.md --- src/boot/store.js | 12 +++++------- src/reduxTypes.js | 4 ++-- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/boot/store.js b/src/boot/store.js index 4b3454c9a45..a103f75859a 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -90,13 +90,11 @@ export const cacheKeys: Array<$Keys> = [ function dropCache(state: GlobalState): $Shape { const result: $Shape = {}; storeKeys.forEach(key => { - // $FlowFixMe[incompatible-indexer] - // $FlowFixMe[incompatible-exact] - // $FlowFixMe[prop-missing] - // $FlowFixMe[incompatible-variance] - // $FlowFixMe[incompatible-type-arg] - /* $FlowFixMe[incompatible-type] - This is well-typed only because it's the same `key` twice. */ + /* $FlowFixMe[cannot-write] + This is well-typed only because it's the same `key` twice. It seems + like we should have to suppress an error about not having that + guarantee, in addition to the more minor-looking `cannot-write`. Not + sure why we don't. */ result[key] = state[key]; }); return result; diff --git a/src/reduxTypes.js b/src/reduxTypes.js index 7ce5f0f87c6..aab34a8debc 100644 --- a/src/reduxTypes.js +++ b/src/reduxTypes.js @@ -329,7 +329,7 @@ export type UsersState = User[]; * See in particular `discardKeys`, `storeKeys`, and `cacheKeys`, which * identify which subtrees are persisted and which are not. */ -export type GlobalState = {| +export type GlobalState = $ReadOnly<{| accounts: AccountsState, alertWords: AlertWordsState, caughtUp: CaughtUpState, @@ -355,7 +355,7 @@ export type GlobalState = {| userGroups: UserGroupsState, userStatus: UserStatusState, users: UsersState, -|}; +|}>; /** A selector returning TResult, with extra parameter TParam. */ // Seems like this should be OutputSelector... but for whatever reason, From df298338b6c6f964ebda0b30344c9b5840233241 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 30 Apr 2021 15:12:21 -0400 Subject: [PATCH 14/14] storage: Start testing several pieces of the Redux storage logic at once. --- src/boot/__tests__/storage-test.js | 89 ++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 src/boot/__tests__/storage-test.js diff --git a/src/boot/__tests__/storage-test.js b/src/boot/__tests__/storage-test.js new file mode 100644 index 00000000000..19d4194e87d --- /dev/null +++ b/src/boot/__tests__/storage-test.js @@ -0,0 +1,89 @@ +/* @flow strict-local */ +import invariant from 'invariant'; + +import * as eg from '../../__tests__/lib/exampleData'; +import objectEntries from '../../utils/objectEntries'; +import type { GlobalState } from '../../types'; +import ZulipAsyncStorage from '../ZulipAsyncStorage'; +import { stringify, parse } from '../replaceRevive'; + +const getRoundTrippedStateValue = async , V: $Values>( + key: K, + value: V, +): Promise => { + // 1: Serialize a "live" object; e.g., a ZulipVersion instance. + const stringifiedValue = stringify(value); + + // 2: Write to storage. Parts of this are mocked; so, stress-test + // those mocks: + // - Compression via TextCompressionModule on Android + // - AsyncStorage: the library provides a mock implementation + // that writes to a variable instead of to the disk + await ZulipAsyncStorage.setItem(key, stringifiedValue); + + // 3: Read from storage. Parts of this are mocked; so, stress-test + // those mocks: + // - Decompression via TextCompressionModule on Android + // - AsyncStorage: the library provides a mock implementation + // that reads from a variable instead of from the disk + const valueFromStorage = await ZulipAsyncStorage.getItem(key); + invariant(valueFromStorage != null, 'valueFromStorage is not null/undefined'); + + // 4: "revive", e.g., a ZulipVersion instance. + const parsedValueFromStorage = parse(valueFromStorage); + + // $FlowIgnore[incompatible-cast] + return (parsedValueFromStorage: V); +}; + +const getRoundTrippedState = async (globalState: GlobalState): Promise => { + const entries = await Promise.all( + objectEntries(globalState).map( + // eslint-disable-next-line flowtype/generic-spacing + async , V: $Values>([key: K, value: V]): Promise< + [K, V], + > => [ + // $FlowIgnore[incompatible-cast] + (key: K), + // $FlowIgnore[incompatible-cast] + (await getRoundTrippedStateValue(key, value): V), + ], + ), + ); + // $FlowIgnore[incompatible-indexer] + return Object.fromEntries(entries); +}; + +/** + * Test several pieces of the Redux storage logic at once. + * + * We're counting on a lot of our Redux state to round-trip through + * storage. Jest can't realistically test everything in this process + * -- in particular, the storage itself, and any native code involved + * -- but, in the places where it can't, at least it can stress-test + * the mocks we've written and use elsewhere, to make sure they're + * reasonably faithful. + * + * `eg.plusReduxState` represents standard example data, though not + * the entire range of possible data; see its jsdoc. Still, we'll want + * to know about any failures to round-trip that arise from changes to + * that. + */ +test('`eg.plusReduxState` round-trips through storage', async () => { + const stateBefore = eg.plusReduxState; + // The `GlobalState` annotation seems to guide Flow to better error + // messages; without it, Flow seems to get confused by the `await`. + const stateAfter: GlobalState = await getRoundTrippedState(stateBefore); + + // `AvatarURL` instances are generally lazy with their `new URL` + // computations; they have a stateful piece that might be a string + // or a `URL` object. We could figure out how to teach Jest that + // that difference doesn't matter, but for now, just normalize both + // states by "waking up" their `AvatarURL`s. + [stateBefore, stateAfter].forEach(s => { + s.users.forEach(u => u.avatar_url.get(100)); + s.messages.forEach(m => m.avatar_url.get(100)); + }); + + expect(stateAfter).toEqual(stateBefore); +});