Page MenuHomePhabricator

[lib] Introduce fetchPendingUpdates action

Authored by ashoat on Sat, Jun 8, 3:44 PM.
Referenced Files
F2030881: D12366.id41179.diff
Tue, Jun 18, 12:16 AM
F2030870: D12366.id41189.diff
Tue, Jun 18, 12:16 AM
Unknown Object (File)
Mon, Jun 10, 5:03 AM
Unknown Object (File)
Mon, Jun 10, 5:01 AM
Unknown Object (File)
Mon, Jun 10, 4:57 AM



Most of the work here was in the reducers. Flow had very peculiar behavior, where it sometimes struggled to narrow types correctly. Where possible I avoided needed to factor things out by adding the new action types to the same top-level conditionals. If that wasn't possible, I would either factor out the logic into a function (if sufficiently complex) or copy-paste if it was really simple. When factoring out into a function, I sometimes opted for simpler parameters that would require a bit more copy-paste at the callsite.

Depends on D12365

Test Plan

In combination with later diffs, I tested as follows:

  1. I created a socket crash loop on a physical iOS device using @inka's create-many-threads-to-trigger-crash-loop.js script (see ENG-8090
  2. I confirmed that the socket was unable to connect prior to my diff stack
  3. I confirmed that after applying my diff stack, the SUCCESS action was dispatched, and the socket was able to connect afterwards

Diff Detail

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

ashoat requested review of this revision.Sat, Jun 8, 4:17 PM
tomek added inline comments.
280–281 ↗(On Diff #41143)

Shouldn't we also set actualizedCalendarQuery like in fullStateSyncActionType?

326 ↗(On Diff #41143)

Should we include actualizedCalendarQuery like in incrementalStateSyncActionType?

430–439 ↗(On Diff #41143)

I wondering if this approach is maintainable. By having a return statement here we're introducing a relationship between action ordering - this action has to be placed below action.type === fetchPendingUpdatesActionTypes.success && action.payload.type === stateSyncPayloadTypes.FULL and it might be hard to notice that during a refactoring. How about deleting a return statement here and letting the reducer process the next actions?

This revision is now accepted and ready to land.Mon, Jun 10, 4:45 AM
280–281 ↗(On Diff #41143)

No, calendarQuery is not part of this action payload. My thinking is that we should set the calendarQuery when the socket is connected, but we don't need to do it when fetching pending updates. This allowed me to simplify the code for the endpoint a little bit.

430–439 ↗(On Diff #41143)

I'll make this change

This revision was landed with ongoing or failed builds.Mon, Jun 10, 2:03 PM
This revision was automatically updated to reflect the committed changes.