Repository navigation
[Tracking] Implement JSON_PATCH() in Onyx.merge() in react-native-onyx(native) #13972
Description
Design Doc: https://docs.google.com/document/d/1Aw3gB8ksT5b1xGBtoAKSPR5pcBXHh9yjmRWv8GGdbXM/edit
Proposal:
Move manual merging to LocalForage, since in SQLiteStorage this is already done with JSON_PATCH
Problem:
Onyx is our low level state management layer. We need this layer to be as fast as possible on native devices as everything the user sees and does is driven by this layer. At the moment when data is merged into Onyx we perform a series of inefficient steps:
- In the past we had to solve a race condition related to our asynchronous storage providers (AsyncStorage and LocalForage) when making updates in a single tick of the event loop. To solve this, we had to queue partial updates into a batch, get the most up to date data from storage, and then merge the queue into the existing data and replace the entire key.
- On native, this requires that we potentially pull large objects out of device storage and send them across React Native bridge.
- We must also serialize and deserialize this data which can be an expensive task to perform especially on native.
Solution:
To properly separate logic for native vs. web, we should have different merge logic for each data provider. My solution would be to move the manual merge to LocalForage.js’s merge function and keep JSON_PATCH in case of conflicts in SQLiteStorage.js
Tasks:
- Post Proposal (Problem/Solution) in #expensify-open-source
- Link to P/S: https://expensify.slack.com/archives/C01GTK53T8Q/p1672862201064419
- Post P/S in this issue description
- Email
strategy@expensify.comwith Proposal - Fill out High-level overview of the problem, Timeline, and Terminology section
- Email
strategy@expensify.com(same email chain as before) with link to Design Doc - Host a pre-design meeting in
#expensify-open-sourceto discuss any necessary details in public before filling out the High-level of proposed solution section - Fill out the High-level of proposed solution section
- Email
stategy@expensify.comagain with links to the doc and pre-design conversation in Slack - Add
DesignDocReviewlabel to get the High-level of proposed solution section reviewed - Respond to any questions or concerns and bring up blockers in Slack to get consensus if necessary
- Confirm that the doc has the minimum necessary number of reviews before proceeding
- Host another pre-design meeting in
#expensify-open-sourceto ask for engineering feedback on the technical solution. - Fill out the Detailed implementation of the solution and related sections.
- Re-add the
DesignDocReview - Respond to any questions or concerns and bring up blockers in Slack to get consensus if necessary
- Confirm that the doc has the minimum necessary number of reviews before proceeding
- Email
strategy@expensify.comone last time to let them know the Design Doc is moving into the implementation phase - Implement the changes
Activity
chrispader commentedon Jan 4, 2023
melvin-bot commentedon Jan 9, 2023
@greg-schroeder Eep! 4 days overdue now. Issues have feelings too...
melvin-bot commentedon Jan 11, 2023
@greg-schroeder 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
melvin-bot commentedon Jan 13, 2023
@greg-schroeder Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!
melvin-bot commentedon Jan 17, 2023
@greg-schroeder 12 days overdue. Walking. Toward. The. Light...
greg-schroeder commentedon Jan 17, 2023
I've been out with COVID, apologies. Reviewing today.
72 remaining items
chrispader commentedon May 20, 2023
Working on the last pieces of code right now, so this should be ready for the (hopefully last) review tomorrow or by Monday! 🚀🥳
chrispader commentedon May 22, 2023
@marcaaron The PR is ready for review!
marcaaron commentedon Jun 26, 2023
Looks like we had deviated from the original proposal a bit. I left some comments about the introduction of a queue and offered a simpler solution. Waiting to see changes or to hear what we think about the feedback. @chrispader please let me know when the PR is ready for re-review by leaving an "Updated" comment on the PR or in this issue. This one has been open for a while and it would be good to get it closed out.
chrispader commentedon Jul 12, 2023
@marcaaron the PR with the new proposal is ready for review! 🥳
marcaaron commentedon Jul 13, 2023
Awesome, thanks! I reviewed it today, looks great !
marcaaron commentedon Jul 18, 2023
Onyx PR merged 🎉
chrispader commentedon Jul 19, 2023
@marcaaron created a PR for the onyx version update here
melvin-bot commentedon Aug 21, 2023
This issue has not been updated in over 15 days. @chrispader, @marcaaron eroding to Monthly issue.
P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!
marcaaron commentedon Aug 22, 2023
This was quietly completed without fanfare 😄 Thanks for sticking with it @chrispader. I think we can close it out now.