Skip to content
This repository has been archived by the owner on Oct 4, 2023. It is now read-only.

Add RTK listenerMiddleware #2828

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add RTK listenerMiddleware #2828

wants to merge 2 commits into from

Conversation

sliptype
Copy link
Contributor

@sliptype sliptype commented Feb 13, 2023

Description

An example of using rtk listenerMiddleware instead of sagas. Some nice benefits:

  • Simpler syntax, no generator functions
  • Not compiled to switch statements, debugging is easier

    While looking into this I realized that metro-react-native-babel-preset includes @babel/plugin-transform-async-to-generator so async functions get transformed to generators regardless. However, hermes supports async/await out of the box, so maybe it would be wise to remove this plugin (could have a perf benefit)

Just a draft for now, would love to hear people's thoughts!

Dragons

Is there anything the reviewer should be on the lookout for? Are there any dangerous changes?

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.

How will this change be monitored?

For features that are critical or could fail silently please describe the monitoring/alerting being added.

Feature Flags

Are all new features properly feature flagged? Describe added feature flags.

@sliptype sliptype requested a review from a team February 13, 2023 16:55
@sliptype sliptype requested review from Kyle-Shanks and removed request for a team February 13, 2023 16:55
@sliptype sliptype requested review from a team and removed request for Kyle-Shanks February 13, 2023 16:55
@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/sk-listener-middleware

@piazzatron
Copy link
Contributor

omg

Copy link
Contributor

@piazzatron piazzatron left a comment

Choose a reason for hiding this comment

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

Hype!! Maybe worth setting this up on web as well? Have we thought at all about how shared/common listeners will work?

export const addListeners = (startListening: AppStartListening) => {
startListening({
actionCreator: setTheme,
effect: async function setThemeEffect(action, listenerApi) {
Copy link
Contributor

Choose a reason for hiding this comment

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

curious, is there a reason this is a named fn instead of an async (action, listenAPI) => ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes the callstack nicer when debugging!

@sliptype
Copy link
Contributor Author

Hype!! Maybe worth setting this up on web as well? Have we thought at all about how shared/common listeners will work?

Yeah I def think worth it for web too. Shared listeners should be similar to how we do sagas currently, can be defined in common and then a root level file in web/mobile can call each listener file with the appropriate startListening

Copy link
Contributor

@dylanjeffers dylanjeffers left a comment

Choose a reason for hiding this comment

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

I absolutely love these changes! so this is just RTK addition, not a RTK-Query thing yeah? if so id say lets def move forward with this generally. itll probably cover 90% of usecases and be much simpler to understand. Do you think the queue stuff could theoretically be built with the middleware, or is that a large enough of a process we'd need a "saga" like approach? guessing with all the cancelleation and minutae it would still be needed? potentially though we could still use listener middleware for over 50% of current offline stuff though

@sliptype
Copy link
Contributor Author

sliptype commented Feb 13, 2023

I absolutely love these changes! so this is just RTK addition, not a RTK-Query thing yeah? if so id say lets def move forward with this generally. itll probably cover 90% of usecases and be much simpler to understand

Yes exactly! I'm honestly just a little hesitant to bifurcate the codebase further though lol

And yeah cancellation is one of the cases it doesn't support, but offline is one of the few/only places we need that

@dylanjeffers
Copy link
Contributor

I absolutely love these changes! so this is just RTK addition, not a RTK-Query thing yeah? if so id say lets def move forward with this generally. itll probably cover 90% of usecases and be much simpler to understand

Yes exactly! I'm honestly just a little hesitant to bifurcate the codebase further though lol

yeah i think we should figure out what we are using for fetch layer first, but this changes that calculus

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants