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); 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/ZulipAsyncStorage.js b/src/boot/ZulipAsyncStorage.js index bfabe1e3440..fb92672eac1 100644 --- a/src/boot/ZulipAsyncStorage.js +++ b/src/boot/ZulipAsyncStorage.js @@ -4,8 +4,9 @@ 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 = await AsyncStorage.getItem(key); + 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 // compression to a version with compression. So we need to detect that. @@ -22,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 @@ -36,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(undefined, 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 new file mode 100644 index 00000000000..3b8684474a6 --- /dev/null +++ b/src/boot/__tests__/ZulipAsyncStorage-test.js @@ -0,0 +1,148 @@ +/* @flow strict-local */ +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!'; + const value = '123!'; + + // 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); + + 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('AsyncStorage.setItem called correctly', async () => { + await run(); + expect(asyncStorageSetItemSpy).toHaveBeenCalledTimes(1); + expect(asyncStorageSetItemSpy).toHaveBeenCalledWith( + key, + Platform.OS === 'ios' ? value : await NativeModules.TextCompressionModule.compress(value), + ); + }); + }); + + 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): Promise => { + throw new Error(); + }); + }); + afterAll(() => { + // $FlowFixMe[cannot-write] Make Flow understand about mocking. + AsyncStorage.setItem = globalMock; + }); + + test('rejects correctly', async () => { + await expect(run()).rejects.toThrow(Error); + }); + }); +}); + +describe('getItem', () => { + const key = 'foo!'; + const value = '123!'; + + // 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); + + 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); + }); + }); + + 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): Promise => { + throw new Error(); + }); + }); + afterAll(() => { + // $FlowFixMe[cannot-write] Make Flow understand about mocking. + AsyncStorage.getItem = globalMock; + }); + + test('rejects correctly', async () => { + await expect(run()).rejects.toThrow(Error); + }); + }); + + 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`)).rejects.toThrow( + `No decompression module found for format ${unknownHeader}`, + ); + }); + }); + } +}); + +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); + }); +}); 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); +}); 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/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) { ; /** A selector returning TResult, with extra parameter TParam. */ // Seems like this should be OutputSelector... but for whatever reason, diff --git a/src/third/redux-persist/createPersistor.js b/src/third/redux-persist/createPersistor.js index feb24a434f8..09216e86b57 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 @@ -72,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/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..2a06bdc568c 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,16 +15,22 @@ 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 + 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) + return } let persistKeys = allKeys.filter((key) => key.indexOf(keyPrefix) === 0).map((key) => key.slice(keyPrefix.length)) @@ -34,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/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 } 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 + } })) } }