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

Networking improvements #31

Merged
merged 28 commits into from
Jun 22, 2023
Merged

Networking improvements #31

merged 28 commits into from
Jun 22, 2023

Conversation

blazlew
Copy link
Collaborator

@blazlew blazlew commented Jun 14, 2023

Description

This partially resolves #30 . From the requirements list this pr is missing caching. Outside requirements, but still related it's missing RemoteData improvements.
I also resolved some of issues that I discovered on the go and blocked me from moving forward.

Changes

  • replaced sagas with typed sagas / typed backend responses
  • fixed found post rn upgrade leftovers
  • fixed / improved android build
  • fixed test setup
  • added persistence for redux (mmkv + encrypted storage for sensitive data)
  • added authentication logic
  • moved some code around

Considerations

  • I introduced one lint error, that seem to be broken (makeRequest function line 66)
  • I just realised it would be nice to mention added flipper plugins in readme
  • not that many tests ⏱️
  • setApiAuthConfig saga seem to me not so bulletproof. An observer of particular redux field would work better here, but this is not the saga's way? the idea originates from another place where I placed such code in separate mini-middleware - it was detatched from sagas if someone prefers not to use them.
  • RehydrateAction type is not 100% accurate, it assumes that we persist reducers only at the top level
  • tests prints some console.logs, I left it on purpose - we're gonna handle it once we add a proper Logger.

Demo

Simulator.Screen.Recording.-.iPhone.SE.3rd.generation.-.2023-06-14.at.22.21.24.mp4

@blazlew blazlew requested a review from sharnik June 14, 2023 20:29
Copy link
Contributor

@szymonkoper szymonkoper left a comment

Choose a reason for hiding this comment

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

Generally, I like the direction of networking changes. It looks better than before. 👍 I left a few comments

Please next time try to do smaller PRs. 🥺 What really is in scope of “Networking improvements” could be as well split into few smaller PRs to be reviewed and merged in steps. Additionally, there are some things that are not related to “Networking improvements” that could be done in a separate PR.

@blazlew
Copy link
Collaborator Author

blazlew commented Jun 19, 2023

Generally, I like the direction of networking changes. It looks better than before. 👍 I left a few comments

Please next time try to do smaller PRs. 🥺 What really is in scope of “Networking improvements” could be as well split into few smaller PRs to be reviewed and merged in steps. Additionally, there are some things that are not related to “Networking improvements” that could be done in a separate PR.

Yeah, sorry for that, It was partially based on another thing which I had to prove in a broader context. Aaand it got a little bit out of hand.

@blazlew blazlew requested a review from szymonkoper June 19, 2023 18:15
Copy link
Contributor

@szymonkoper szymonkoper left a comment

Choose a reason for hiding this comment

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

Looks good 👍 All issues resolved or extracted to a separate topics

@blazlew blazlew merged commit e659fab into develop Jun 22, 2023
@blazlew blazlew deleted the networking-improvements branch June 22, 2023 07:55
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.

Update networking setup
2 participants