Skip to content

Commit

Permalink
storage: An absent native module might be null, not only undefined.
Browse files Browse the repository at this point in the history
Fixes #3588.

On Android, we use a small native module of our own,
TextCompressionModule, to compress the Redux data we store in RN's
AsyncStorage facility.  On iOS, RN's AsyncStorage implementation
doesn't impose the same draconian size limits as on Android, so we
haven't made a priority of writing a similar native module there.  Our
JS-side wrapper code therefore checks to see if TextCompressionModule
is present before trying to use it.

Up through RN v0.57, if a native module didn't exist then the value
found when looking for it would be `undefined`.  We wrote our
conditional accordingly to look for `undefined`.

Starting with RN v0.58 -- specifically with the commits leading up to
v0.58.0~281, in which the code managing the JS engine was switched
over to the new JSI from directly using JSC -- this value became
`null`.  Our code would see it wasn't `undefined`, and blithely
attempt to invoke it -- with the result that on iOS where the module
didn't actually exist, all attempts to save data failed.  The most
conspicuous effect of this was that on a fresh install, we'd forget
the user's login information as soon as the app was restarted.

When loading data, on the other hand, we'd always see on iOS that the
raw stored data was not in the compressed format -- and so we'd skip
attempting to decompress it, without even looking at whether the
module was present.  That's why we could always read existing data.

Most insidiously, on the newer RN versions, while the value of a
missing native module is `null` in the default configuration of both a
release or a debug build... if we attempt remote JS debugging, then it
goes back to `undefined`.  This greatly lengthened debugging -- first
because it made the bug simply unreliable to reproduce until I pinned
down that that was the variable, and then because it meant debugging
had to proceed without the debugger and console.
  • Loading branch information
gnprice committed Aug 19, 2019
1 parent a99336f commit 5cf7f10
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/boot/ZulipAsyncStorage.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export default class ZulipAsyncStorage {
if (result !== null && result.startsWith('z')) {
const header = result.substring(0, result.indexOf('|', result.indexOf('|') + 1) + 1);
if (
NativeModules.TextCompressionModule !== undefined
NativeModules.TextCompressionModule
&& header === NativeModules.TextCompressionModule.header
) {
result = await NativeModules.TextCompressionModule.decompress(result);
Expand All @@ -48,7 +48,7 @@ export default class ZulipAsyncStorage {
}

static async setItem(key: string, value: string, callback: ?(error: ?Error) => void) {
if (NativeModules.TextCompressionModule === undefined) {
if (!NativeModules.TextCompressionModule) {
return AsyncStorage.setItem(key, value, callback);
}
return AsyncStorage.setItem(
Expand Down

1 comment on commit 5cf7f10

@cyphase
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Programming sucks sometimes. 😭

Please sign in to comment.