You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is something we've talked about in a number of places. Filing this to have a single thread to cross-reference.
The notifications we show on Android are driven by Android-native code, written in Kotlin and running on the JVM, unlike the bulk of our code which runs in JS in the React Native environment. (On iOS we don't involve any of our client-side code at all; at some point we'll build fancier notifications there too, and that will similarly involve iOS-native code written in Swift.)
As things stand, that code doesn't have a way to look at any of the data stored by the main app code. There are a number of things we'd be much better able to do for the notifications if it did; in particular, just from reading that data without writing to it:
But we'll leave that out of scope for this issue.)
There's no fundamental reason we can't do this. The data is stored in SQLite, which is straightforward to access from Kotlin (and in fact is the standard recommended way to have a database in an Android app.) Within there, it's compressed, which we actually handle in Java in the first place (though there's a tiny container format around that which we've written in JS.) Then the data inside that is a JSON-encoded blob.
One thing that may cause a bit of mess is where our data structures aren't plain JS objects and arrays, i.e. the things that JSON represents directly. For example the data #5116 wants is (mainly) in state.users, and although that is currently an array, it should really be an Immutable.Map. We encode and decode these in src/boot/replaceRevive.js. Reading this data from Kotlin will mean another place that needs to know the conventions that are otherwise encapsulated inside that file. I don't think this will be a big deal, though; that format doesn't change often.
Another source of mess will be where the existing representation isn't well factored in the data structures, and our JS code relies on caching selectors to present a smoother interface. #5116 provides an example of this too: the data it actually needs is not only in state.users but also state.realm.crossRealmBots and state.realm.nonActiveUsers. The bulk of our code doesn't notice this fragmentation because it goes through getAllUsers, which agglomerates the three (and more directly, through getAllUsersById or getAllUsersByEmail, which further turn the data into a nice map form.) Kotlin code won't be able to invoke those, so it'll need to duplicate their functionality, and in particular their awareness of the peculiar scattering of the data across three places. This will not ultimately be a big deal either: we don't generally grow new cases of this, and we should perhaps just take this as impetus to clean up the existing ones.
A more lastingly sticky point may be the issue of migrations. When the user upgrades the app -- particularly when it happens automatically -- they may get a notification before they've happened to use the new version of the app in the foreground. That means the JS-side code won't have run yet, which is where our migrations live, but we'll be running the new version's Kotlin code that wants to look at the data. Options include:
We could move the migrations to the platform-native side, so that the notifications code can run it directly when a migration is needed. I'm wary of this because it'd require having two copies -- the one in Kotlin, and the one that runs on iOS (whether that be in Swift, or initially still be in JS.) Migrations are potentially tricky code that doesn't get effectively type-checked or (at present, anyway) unit-tested (edit: since Add tests for all our migrations; fix an old broken one #5190 they are unit-tested, at least), and doesn't naturally get much integration testing from just using the app. They have a pretty high risk for bugs, and a major mitigating factor there in the status quo is that they are a very small amount of code which we can read closely.
We could have the Kotlin code that reads the data include backward-compatibility logic for old schemas. This is a bit like having migrations there, but the big difference is that it'd be adapting the data only for its own use, not writing a migrated version back, so the impact of a bug is greatly reduced. Also, as long as it's only reading small portions of the overall schema (which would be true for all our existing intended use cases), most migrations wouldn't require any Kotlin-side logic at all. I think this is a plausible option.
We could have the Kotlin code look to see if the database needs a migration (i.e. if its schema version is up to date with the current one the code expects), and if so then get the JS code to do it. This would mean starting up a headless JS task in the background; it might take a few seconds (mainly to start up the JS engine), but would only happen once per upgrade so that should be fine. I think this is also a plausible option.
This may be easiest to sequence after #4841, which will rearrange how we store our data; and #5005, which will mean keeping around server data for all accounts, not just one active account at a time, and will also cause some rearrangement of the data.
The text was updated successfully, but these errors were encountered:
I have a draft branch with a working proof of concept, illustrated in gnprice/zulip-mobile@edf31dadd: one writes (in Kotlin) AppDataRepository(context).account(realmUrl, userId), and gets back an Account?, where Account is a data class that has among other fields a zulipFeatureLevel: Int?.
The main open task before the branch can be merged is that we should have some appropriate tests for it: integration tests between our JS and Kotlin code, to ensure that our Kotlin can read the data that our JS writes. I'll file a separate issue for that… ok, filed JS/Kotlin integration tests #5589.
This is something we've talked about in a number of places. Filing this to have a single thread to cross-reference.
The notifications we show on Android are driven by Android-native code, written in Kotlin and running on the JVM, unlike the bulk of our code which runs in JS in the React Native environment. (On iOS we don't involve any of our client-side code at all; at some point we'll build fancier notifications there too, and that will similarly involve iOS-native code written in Swift.)
As things stand, that code doesn't have a way to look at any of the data stored by the main app code. There are a number of things we'd be much better able to do for the notifications if it did; in particular, just from reading that data without writing to it:
(Further features that would benefit from writing to that data include
But we'll leave that out of scope for this issue.)
There's no fundamental reason we can't do this. The data is stored in SQLite, which is straightforward to access from Kotlin (and in fact is the standard recommended way to have a database in an Android app.) Within there, it's compressed, which we actually handle in Java in the first place (though there's a tiny container format around that which we've written in JS.) Then the data inside that is a JSON-encoded blob.
One thing that may cause a bit of mess is where our data structures aren't plain JS objects and arrays, i.e. the things that JSON represents directly. For example the data #5116 wants is (mainly) in
state.users
, and although that is currently an array, it should really be anImmutable.Map
. We encode and decode these insrc/boot/replaceRevive.js
. Reading this data from Kotlin will mean another place that needs to know the conventions that are otherwise encapsulated inside that file. I don't think this will be a big deal, though; that format doesn't change often.Another source of mess will be where the existing representation isn't well factored in the data structures, and our JS code relies on caching selectors to present a smoother interface. #5116 provides an example of this too: the data it actually needs is not only in
state.users
but alsostate.realm.crossRealmBots
andstate.realm.nonActiveUsers
. The bulk of our code doesn't notice this fragmentation because it goes throughgetAllUsers
, which agglomerates the three (and more directly, throughgetAllUsersById
orgetAllUsersByEmail
, which further turn the data into a nice map form.) Kotlin code won't be able to invoke those, so it'll need to duplicate their functionality, and in particular their awareness of the peculiar scattering of the data across three places. This will not ultimately be a big deal either: we don't generally grow new cases of this, and we should perhaps just take this as impetus to clean up the existing ones.A more lastingly sticky point may be the issue of migrations. When the user upgrades the app -- particularly when it happens automatically -- they may get a notification before they've happened to use the new version of the app in the foreground. That means the JS-side code won't have run yet, which is where our migrations live, but we'll be running the new version's Kotlin code that wants to look at the data. Options include:
or (at present, anyway) unit-tested(edit: since Add tests for all our migrations; fix an old broken one #5190 they are unit-tested, at least), and doesn't naturally get much integration testing from just using the app. They have a pretty high risk for bugs, and a major mitigating factor there in the status quo is that they are a very small amount of code which we can read closely.This may be easiest to sequence after #4841, which will rearrange how we store our data; and #5005, which will mean keeping around server data for all accounts, not just one active account at a time, and will also cause some rearrangement of the data.
The text was updated successfully, but these errors were encountered: