This differential updates SQLiteContextProvider to use new redux action and set store loaded attribute in redux
Details
Details
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
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-2137
- Lint
No Lint Coverage - Unit
No Test Coverage
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? |
| 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. |
| 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 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.
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 |
| 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 |
| native/data/sqlite-context-provider.js | ||
|---|---|---|
| 88–96 ↗ | (On Diff #18780) |
Yeah, that's what I meant. I don't think this will be too confusing - quick check in reducer would make it clear.
I was comparing the first implementation of this code with the current one. |
Comment Actions
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).
Comment Actions
After short discussion with @tomek we agreed on SQLiteDataHandler. Provider should be reserved for components that provide some value for other components.