Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add redux store for handling current running connector and logs #516

Merged
merged 11 commits into from
Jan 24, 2023

Conversation

Ldoppea
Copy link
Member

@Ldoppea Ldoppea commented Dec 23, 2022

We want to keep track of the current running connector so we can
prevent multiple connectors to run at the same time. Also we want to
persist this info so we can mark the connector as failed after a
restart if the app crashed during its execution

We also want to persist connectors logs so we can still send them to the
cozy-stack if the app crashes before being able to send them

This commits add the foundation for managing connectors through redux

The complete logic is not implemented as we only implement the store
container with all the redux logic


Usage:

import { useConnectorState } from 'src/providers/ConnectorState/ConnectorStateProvider'

// in component's logic
const { addLog, currentRunningConnector, setCurrentRunningConnector } = useConnectorState()

const doSomething = () => {
  setCurrentRunningConnector('some_slug')
  addLog('some logs')
}

TODO:

  • Check how to clear the store on logout (⚠️ connector logs must be sent to stack before clearing the store)
  • Verify if the currentRunningConnector should be persisted or not

@Ldoppea Ldoppea changed the title Feat/connector redux persist feat: Add redux store for handling current running connector and logs Dec 23, 2022
@acezard
Copy link
Contributor

acezard commented Dec 26, 2022

Are we sure we want to use Redux here? It's very verbose, maybe we can look at lighter alternatives?

@Ldoppea
Copy link
Member Author

Ldoppea commented Dec 26, 2022

Are we sure we want to use Redux here? It's very verbose, maybe we can look at lighter alternatives?

What alternative do you have in mind?

Here we chose react-redux because we want a state management that automatically persist its data, and because it is already used by cozy-client (so we keep the same stack).

@Ldoppea Ldoppea force-pushed the feat/connector_redux_persist branch from b3cac7f to ef1bdb1 Compare December 28, 2022 17:07
@Crash--
Copy link
Contributor

Crash-- commented Dec 29, 2022

@Ldoppea We should talk about this. If you need slice within redux, you should combine the one from cozy-client and the one you want to add and then make a setStore call to cozy-client. https://github.com/cozy/cozy-banks/blob/e1a2b5435f4be2d55a744190aafc243005c857f1/src/main.jsx#L118 for instance (to be able to do that, when you initiliaze cozy-client, you should set store to false (https://github.com/cozy/cozy-banks/blob/e1a2b5435f4be2d55a744190aafc243005c857f1/src/ducks/client/web.js#L41)

You should also be aware, that we have a new API from CozyClient to persist queries. See cozy/cozy-notes#327 for how to handle it. (I know this is not what you need for now, but maybe it can provide you some guidance / vision)

@Ldoppea Ldoppea force-pushed the feat/connector_redux_persist branch 5 times, most recently from 05976a7 to b72f9fa Compare January 13, 2023 17:38
@Ldoppea Ldoppea marked this pull request as ready for review January 13, 2023 17:56
@Ldoppea
Copy link
Member Author

Ldoppea commented Jan 13, 2023

Latest decision:

  • We won't merge the store with the cozy-client one for now. But we expect to do it in a later PR

This PR is ready for review

Warning
Do not review the logic inside of sendConnectorsLogs() and useConnectors > doAddLog(), those are demo codes and are expected to be replaced in #534

middleware: getDefaultMiddleware =>
getDefaultMiddleware({
serializableCheck: {
ignoredActions: [FLUSH, REHYDRATE, PAUSE, PERSIST, PURGE, REGISTER]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand this need. Is it because of TS?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK next time I'll read the commit message d11100b

So now my question is: why do we use redux-tookit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woups I missed this notification.

I used it because it is the official recommendation: https://redux.js.org/introduction/getting-started

We want to keep track of the current running connector so we can
prevent multiple connectors to run at the same time. Also we want to
persist this info so we can mark the connector as failed after a
restart if the app crashed during its execution

We also want to persist connectors logs so we can still send them to the
cozy-stack if the app crashes before being able to send them

This commits add the foundation for managing connectors through redux

The complete logic is not implemented as we only implement the store
container with all the redux logic

Persistance will be implemented in the next commits
This commits add the foundation for managing connectors logs

The complete logic is not implemented as we only implement the hooks
and methods that should be called when adding a log and when we want to
process all of them

The logic should be implemented in a future PR
This commit implement redux persistance through the app's AsyncStorage

This will allow to recover some info about running connectors after an
app crash

This info will be about latest running connector and all pending logs
that should be sent to the cozy-stack
When using redux-persist with redux-toolkit some non-serialisable
actions may be dispatched to the store. This can be the case on app
start when redux-persist tries to rehydrate the store from AsyncStorage

When this happens, an exception is thrown by redux-persist

To prevent this we want to ignore those actions from the store's
middleware

More info:
https://redux-toolkit.js.org/usage/usage-guide#use-with-redux-persist
When we logout the user, we want to check for unprocessed logs and to
send them to the stack

This scenario is possible if the connector did not complete
successfully and if the error was not handled correctly
If the App crashes during a connector execution, some connector logs
may remain in the AsyncStorage

We want to send those logs (if any) on next App boot

Also we can detect if a connector crashed by reading the
currentConnector state. This commit add an entry point for this check,
but we still have to set this state when running a connector and then
replace the `console.error` with a cleaning action (ex: stop jobs)
@Ldoppea Ldoppea force-pushed the feat/connector_redux_persist branch from b72f9fa to 66b8bfe Compare January 24, 2023 12:33
@LucsT LucsT merged commit dd95316 into master Jan 24, 2023
@LucsT LucsT deleted the feat/connector_redux_persist branch January 24, 2023 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants