App: [Discussion] Onyx.connect() design leads to ambiguously defined local values
Problem
Onyx.connect()
updates local variables asynchronously wherever it is used. This has at times lead to incorrect assumptions about when a certain local variable will be “ready” or “not ready” and therefore increases the chances of race conditions (inconsistent results) and confusing time dependencies (do x, but only if y is “ready”).
Why is this important?
While there haven’t been a ton of scenarios where we have badly assumed that some Onyx value is “ready” I have personally encountered enough that I am assuming it is likely to happen again in the future. It is at the very least something we should watch out for.
Solution
There have been a several proposed solutions to this problem:
- synchronous
Onyx.get()
- load all data upfront and then access values in cache
Onyx.isReady()
promise that resolves when the value is ready- Create something like
withOnyx()
for use in actions files so that any actions will automatically get queued until the values are returned. - Create some kind of interface for an “action” and then create them with a factory or class like
function signIn({credentials}) {
// ... do sign in stuff that requires credentials
}
const mapping = {
credentials: {
key: ONYXKEYS.CREDENTIALS,
},
};
const Session = new ActionCollection({
mapping,
exports: [signIn],
});
// Where ActionCollection waits for values and internally
// decorates each exported method with something like
isReady().then(() => exportMethod(this.values));
About this issue
- Original URL
- State: closed
- Created 3 years ago
- Comments: 17 (17 by maintainers)
RxJS could work, but IMO will reduce the people that can work on the project
If we return an object from
connect
we can expose some fields to make the value observable orasObservable
method We can even make Onyx connections observable right now with something likeThough we’re lacking some callbacks like
onError
andonDisconnect
to relay that information to the ObservableSubscriptions use
Onyx.get
internally to start with the latest value - the value that other connections could have changed by now, the way Onyx works is virtually impossible to callOnyx.get
and get an older valueThe only mishap that I can see with people using it is similar to what I’ve shared earlier
So maybe we can enhance connections with a promise to get the best of both worlds
connection.promise
resolves once the connection has finished reading the storage valueconnection.value
is something similar to what we do here:The downside of this is keeping the connection “forever” - Onyx.get allows us to release unused resources
Onyx.getSync
just won’t work - it’ll have the same problem we have with top level connections - has it loaded the data or not - I realised that and ended my comment It’s because we can’t read everything from storage into cache/memory - we can only read the finite items, which excludes conversations Even if we could read everything in memory, why keep it there when only 50% or less is going to be usedIn the cases we’re needing
Onyx.get
it’s not due to reactivity, we useconnect
andwithOnyx
to react to a change We needOnyx.get
when we want to ensure we’re using the latest data through an actionHere’s a practical application where I don’t find anything else suitable but
Onyx.get
: https://github.com/Expensify/App/issues/5987#issuecomment-963471699connect
Onyx.get
is the most optimal way (so far) to deal with storage values that we don’t need in memory all the time It works lazily on demand - if data is already cached great off we go, but it doesn’t force something to linger in memory just in caseActions already deal and wait for promises whether you wait on a http request or on Onyx.get doesn’t make a difference to how the end result is handled
So far I’ve provided sound reasons to why we need
Onyx.get
exposed, but I don’t see anything definitive as to why not and what problems could result by exposing itNow tell me what you like about it ? :trollface:
So yeah, points taken and I agree with a lot of it and mainly tried to come up with something to kick off this conversation that could maybe satisfy our constraints of
A - Not wanting excessive promise chains B - Not wanting to use
Onyx.get()
C - Not wanting to adopt Async/AwaitWe could solve it by having a particular method blocked by the “readiness” of only the things it needs
That might not be a bad thing as we already need to learn
withOnyx
and the application seems really similar - which could make things more consistent. But I’ll admit this does remind me of the initial concerns that others had about using something like Redux and fear of it having “too much boilerplate”. So we should be cautious to avoid heading down a similar path here.I feel like this is OK as long as long as we like whatever solution we come up with. e.g. it would be a lot of work to replace all local variables with
Onyx.get()
and turn all actions into async functions - but the right design will be worth implementing.I think you pretty much solved this with your next comment: https://github.com/Expensify/App/issues/6151#issuecomment-957424645. @roryabraham had a similar idea here.
Yes, but violates constraints A and B
Yes, that is constraint C