[Experiment] NetworkResponseManager

[marcaaron]

Related to: #6820

Details

cc @tgolen @iwiznia just a proof of concept that sets up a few things

• Allows "registering" a callback for a command
• Borrows the .done() and .handle() pattern we are used to in Web-Expensify
• Tests it out on PersonalDetails_Update and makes that command "persistable"

Couple of interesting takeaways...

1. By decoupling the handling from the call to API.PersonalDetails_Update() we lost some context to things that the original caller had e.g. there was a shouldGrowl parameter getting passed to setPersonalDetails() and that parameter is not available to the subscriber. One solution I came up with was to just pass that to the "request" object so it can be stored there and then returned back in the success handler. In this case, that works fine, but wouldn't work with something like a callback function (but maybe we can just call that an "anti-pattern" if we adopt a similar system to this).

2. I picked API.PersonalDetails_Update() because there are two different usages, but only one of them has specific handling. That created an interesting problem because we might run into cases where we want completely different handling depending on the caller. I couldn't think of a clean way to solve that besides passing some kind of key to the request object to tell us that that particular request should have different handling. Not sure if this is really a problem or not, but if we have to do it it seems kind of ugly.

[iwiznia]

I picked API.PersonalDetails_Update() because there are two different usages, but only one of them has specific handling. That created an interesting problem because we might run into cases where we want completely different handling depending on the caller. I couldn't think of a clean way to solve that besides passing some kind of key to the request object to tell us that that particular request should have different handling. Not sure if this is really a problem or not, but if we have to do it it seems kind of ugly.

hmmm yeah, different handling depending on the caller sounds like something we will need, in the end different uses of the same API command for different cases could need different handling, no? So I think we need to support that somehow.

[tgolen]

To solve the problem of different callers, the only thing I thought of is making two separate API methods (API.PersonalDetails_Update and API.PersonalDetails_Update_DeleteAvatar. That way, each API method only has one caller. That's not super ideal, but it does help keep things clean.

I for sure think that the growls are an anti-pattern and that's something that we are forming a more general framework around with the N7 account settings. For those personal detail grows, those should be server errors handled inline in the form, shouldn't they?

I'm not too sure that this fits any of the proposals in the offline doc, since I thought we were going with the "pusher first" approach.

One thing I had talked about with @coleaeason, that I think would resonate with @iwiznia was the reliance on Pusher. Cole was very concerned about it being a single point of failure. Which I think is a fair concern. The idea that I had was something like this:

1. All data gets sent to the app with Pusher
2. All jsonCodes are sent in the HTTP response
3. If Pusher goes down, PHP detects this and switches to sending data to the app in the HTTP response
4. The front-end can then handle responses from either Pusher or from HTTP response
5. No duplicate data is sent to the front-end with this proposal

I think in a world like that, then an event-driven architecture like this would be helpful (so that either Pusher or the HTTP Network lib can publish these API command events.

[tgolen]

What would this be used for?

[marcaaron]

Might not be necessary at all - I just jacked this code from a very minimal PubSub implementation to make this "complete". Probably these subscriptions would be like our many Onyx.connect() calls that do not need to be explicitly "unsubscribed" or "disconnected".

[tgolen]

More of a naming conversation, but this seems pretty confusing and the separation is not really intuitive.

I don't have any other suggestions for this now, but I wanted to pull it out as something to think about.

[marcaaron]

Agree it could be incorporated into network. The separation just helped me compartmentalize the problem and just show off the "new code" we could be adding.

[marcaaron]

Another idea could be to pass a second parameter to both API.Something() and the handler that is specific to the action that is calling the API...

function setPersonalDetails() {
    Onyx.merge(ONYXKEYS.ACTION.SET_PERSONAL_DETAILS, {loading: true, errors: null});
    API.PersonalDetails_Update({...params}, ONYXKEYS.ACTION.SET_PERSONAL_DETAILS);
}

Network.onCommandFinish('PersonalDetails_Update', ONYXKEYS.ACTION.SET_PERSONAL_DETAILS)
    .done(...)
    .handle(...);

That might be a good Onyx key to subscribe to in the UI to get information about whether blocking actions have loading states or error messages. At the moment, we often mingle this information with the data that an action is setting, but maybe some separation there would be good and the ONYXKEYS.ACTION.SOMETHING could match the action name.

[marcaaron]

I for sure think that the growls are an anti-pattern and that's something that we are forming a more general framework around with the N7 account settings. For those personal detail grows, those should be server errors handled inline in the form, shouldn't they?

Yep that is a great next conversation to have. TBH, I just wanted to showcase a way we could generically handle an error using the event driven stuff - less concerned in this step about what the most optimal UX will be and not trying to make a perfect example - just one that adopts the event driven model.

I'm not too sure that this fits any of the proposals in the offline doc, since I thought we were going with the "pusher first" approach.

Good thoughts about Pusher. I guess we can wait to see where that stuff lands. And sounds like we agree there will be some need for an event driven response handling.