Page MenuHomePhabricator

Use new redux action and storeLoaded field
ClosedPublic

Authored by marcin on Nov 23 2022, 6:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 7, 1:26 AM
Unknown Object (File)
Thu, Mar 7, 1:26 AM
Unknown Object (File)
Thu, Mar 7, 1:26 AM
Unknown Object (File)
Tue, Mar 5, 1:16 AM
Unknown Object (File)
Fri, Mar 1, 6:21 PM
Unknown Object (File)
Fri, Mar 1, 6:21 PM
Unknown Object (File)
Fri, Mar 1, 6:21 PM
Unknown Object (File)
Fri, Mar 1, 6:21 PM
Subscribers

Details

Summary

This differential updates SQLiteContextProvider to use new redux action and set store loaded attribute in redux

Test Plan

Initialize debugging session with flipper. Open, close, repoen the app. Send messages, create drafts. Examine that payload on SET_CLIENT_DB_STORE action is always as expected.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

native/data/sqlite-context-provider.js
88–96 ↗(On Diff #18780)

It might be more readable to have a separate action called setStoreLoadedActionType that just sets storeLoaded – what do you think?

tomek requested changes to this revision.Nov 23 2022, 10:09 AM
tomek added inline comments.
native/data/sqlite-context-provider.js
88–96 ↗(On Diff #18780)

Agree, having a separate action will be less noisy. At the same time, the actions may (in theory) set drafts, messages and threads to empty state, so replacing them with separate action doesn't match that.

113–114 ↗(On Diff #18780)

Use shorthands

182 ↗(On Diff #18780)

We're no longer providing a context so maybe instead of always rendering children, we can take a different approach where this component always renders null and is rendered next to the rest of the app? We're using this approach in a lot of other places.

This revision now requires changes to proceed.Nov 23 2022, 10:09 AM
native/data/sqlite-context-provider.js
88–96 ↗(On Diff #18780)

I am not sure what do you mean by "separate".
If by "separate" you mean not to set storeLoaded in response to action that sets SQLite data and dispatch another action for storeLoaded then it is in conflict with this Linear description: https://linear.app/comm/issue/ENG-2138/move-storeloaded-to-redux. The main objective of introducing storeLoaded to redux is to make sure it is set at the same time SQLite data are set.

If by "separate" you actually mean "additional" action to set just storeLoaded if all other fields of the payload are {} then it is correct and does not conflict with Linear. Code might be more readable, but I am afraid it could be confusing for future developers, that might be under impression that setStoreLoadedActionType must be dispatched every time setClientDBStore is dispatched.

At the same time, the actions may (in theory) set drafts, messages and threads to empty state, so replacing them with separate action doesn't match that.

It is not clear to me what kind of issue are you describing here. As far as I understand the code implemented in this differential works correctly, but it needs some readability related refactoring. Could you be more specific which particular redux action might be in conflict with the code above?

native/data/sqlite-context-provider.js
182 ↗(On Diff #18780)

Sure

Use set store loaded action type in sqlite context provider

ashoat added inline comments.
native/data/sqlite-context-provider.js
22 ↗(On Diff #18836)

Now that SQLiteContext has been deleted, we should rename this component. How about SQLiteHandler? Open to other suggestions

tomek added inline comments.
native/data/sqlite-context-provider.js
88–96 ↗(On Diff #18780)

If by "separate" you actually mean "additional" action to set just storeLoaded if all other fields of the payload are {} then it is correct and does not conflict with Linear. Code might be more readable, but I am afraid it could be confusing for future developers, that might be under impression that setStoreLoadedActionType must be dispatched every time setClientDBStore is dispatched.

Yeah, that's what I meant. I don't think this will be too confusing - quick check in reducer would make it clear.

It is not clear to me what kind of issue are you describing here. As far as I understand the code implemented in this differential works correctly, but it needs some readability related refactoring. Could you be more specific which particular redux action might be in conflict with the code above?

I was comparing the first implementation of this code with the current one.
In the first implementation we were dispatching an action that resulted in storeLoaded being set to true and in threads, messages and drafts being set to empty collections.
In this implementation we're only setting the store loaded flag and do not change the content of other collections. But that is ok, because we assume that this action will be dispatched only when the store wasn't loaded from the db yet.
So both implementations in our case work the same, and the current one is more readable.

This revision is now accepted and ready to land.Nov 28 2022, 2:01 AM
native/data/sqlite-context-provider.js
22 ↗(On Diff #18836)

I would suggest SQLiteDataManager or SQLiteDataHandler. I would specify that this component deals exactly with data that is stored inside SQLite, not the database in general. @tomek what do you think?

Actually in my opinion simply SQLiteDataProvider is the best name of this component, since it explains exactly what it does. Manager or Handler could be useful for component that has more general responsibility (it fetches and stores some data as well).

Rename SQLiteContextProvider to SQLiteDataProvider

After short discussion with @tomek we agreed on SQLiteDataHandler. Provider should be reserved for components that provide some value for other components.

This revision was automatically updated to reflect the committed changes.