App: [Discussion] Onyx.connect() design leads to ambiguously defined local values

cc @roryabraham @kidroca

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)

Most upvoted comments

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 or asObservable method We can even make Onyx connections observable right now with something like

function observableConnection(key) {
  return new Observable((subscriber) => {
    Onyx.connect({
      key,
      callback: value => subscriber.next(value);
    });
}

Though we’re lacking some callbacks like onError and onDisconnect to relay that information to the Observable

Subscriptions 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 call Onyx.get and get an older value

The only mishap that I can see with people using it is similar to what I’ve shared earlier

function someAction() {
  Onyx.get(myKey)
      .then(myValue => {
          makeRequest()
              .then(() => /* can't rely on myValue here - you have to get it again */)
      })
}

So maybe we can enhance connections with a promise to get the best of both worlds

// top level connection call
const someConnection = Onyx.connect({
  key: 'myKey',
  callback: ...
});

function someAction() {
  someConnection.promise()
    .then(() => {
       if (someConnection.value == 'go') {
           return makeRequest();
       }
    })
    .then(() => {
       // someConnection.value is kept up to date and can be used here
    });
}

connection.promise resolves once the connection has finished reading the storage value connection.value is something similar to what we do here:

let myValue;
Onyx.connect({
  key: 'myKey',
  callback: val => myValue = value;
});

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 used


In the cases we’re needing Onyx.get it’s not due to reactivity, we use connect and withOnyx to react to a change We need Onyx.get when we want to ensure we’re using the latest data through an action

Here’s a practical application where I don’t find anything else suitable but Onyx.get: https://github.com/Expensify/App/issues/5987#issuecomment-963471699

  • there is also an example of “a creative solution” using connect

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 case

Actions 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 it

This solution seems a bit “greedy”,has unnecessary complexity and will have problems in async scenarios

Now 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/Await

Looks like everything in mapping need to be ready before using any of the exports

We could solve it by having a particular method blocked by the “readiness” of only the things it needs

when someone needs to write an action they’ll need to learn how

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.

applying it requires extra work

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.

actions are async - storage mappings like function signIn({credentials}) represent the correct value at the start of the call. If the action involves a promise we can’t trust the mappings inside a .then block

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.

Well isn’t the above very similar to this

Yes, but violates constraints A and B

One thing that would improve the 2nd example is using async/await

Yes, that is constraint C