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

Experimentation: Action listener middleware #272

Closed
wants to merge 5 commits into from

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Dec 5, 2019

This is some experimentation around a action listener middleware. (See #237)

@markerikson I would love your input on this approach.
I've added it as a standalone middleware creation function, as an option to createSlice, and added development mode runtime validations that add a few sanity checks, most importantly that if a slice has a middleware, it that middleware has been included when the reducer is being used.

This currently has not tests or documentation whatsoever, it's really just a draft.

Also, I haven't tried this out myself... opening up this PR so that I can play around with it in a shiny codesandbox :D

@netlify
Copy link

netlify bot commented Dec 5, 2019

Deploy preview for redux-starter-kit-docs ready!

Built with commit 32c03cc

https://deploy-preview-272--redux-starter-kit-docs.netlify.com

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 5, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 32c03cc:

Sandbox Source
crimson-shadow-3hfbe Configuration
eager-chandrasekhar-po9cj Configuration
rsk-github-issues-example Configuration

@phryneas
Copy link
Member Author

phryneas commented Dec 5, 2019

Okay, little experiment here:
Edit charming-fermat-yw3fk

Also take a look at the console - it warns now if you use the same slice reducer twice, two slices with the same name and (which is why I added that initially) when you use a slice that has a middleware, but you forget adding the middleware.

But it also opens up a new question:
How to react to actions from the same slice?

  • Build their name by hand? (cumbersome)
  • Prefix all actions in the middleware with the slice name? (makes impossible to react to non-slice-actions)
  • add an alternative way of calling, like a callback thisSliceActions => actionListenerMap? This would collide if we added the builder callback style
  • combine both: builder callback with a reference to the current slice's actions. Optionally allow returning a map object that will be used if no builder.addCase has been called?

@joelnet
Copy link

joelnet commented Jan 25, 2020

This experiment looks interesting. I am looking forward to it!

I have also been reviewing rematch which also offers similar and also an interesting API.

One thing that is neat about rematch is how their effects are a single function that takes a dispatch. The count is added onto the dispatch and the dispatch is at a higher level so now each effect function doesn't need to list it in the params.

export const count = {
        /* truncated for brevity */
	effects: dispatch => ({
		// handle state changes with impure functions.
		// use async/await for async actions
		async incrementAsync(payload, rootState) {
			await new Promise(resolve => setTimeout(resolve, 1000))
			dispatch.count.increment(payload)
		},
	}),
}

@phryneas
Copy link
Member Author

Hm... looking at rematch, I'm not a big fan. While their implicit binding may sure save some imports, it takes away a lot of clarity. Also, it might make usage with TS more difficult. While now, custom typing of dispatch is optional for most people unless you have specfic middlewares, Then it would become norm to create types to gap the context boundary.

Also, it adds the assumption that these properties have to exist on dispatch, and if they don't you'll get a runtime error - opposed to the action just "doing nothing", which feels much more sane in the mental image that actions could be viewed as "domain events".
"Event dispatched, nobody reacted" against "event dispatched, app crashed".

The core question is another: do we want to follow this approach at all? Is it worth it? Can we find good solutions for the questions I brought up in #272 (comment) ?

@joelnet
Copy link

joelnet commented Jan 27, 2020

Can we find good solutions for the questions I brought up in #272 (comment) ?

since api is passed in with getState and dispatch, maybe actions could be added to that? or as this.actions.actionFinished().

I think self referencing the slice is fine too, like you did in your example

Actually, I think referencing them might be best (slice1.actions.actionFinished()). That way you could call actions from a different slice.

@phryneas
Copy link
Member Author

since api is passed in with getState and dispatch, maybe actions could be added to that? or as this.actions.actionFinished().

Honestly, I don't see the value of dispatch.slice.name() or something like that over dispatch(slice.actions.name()) which you could even get down to dispatch(actions.name()) or just dispatch(name()) when exporting/using a destructured slice.

In the best case, it may save a few characters (only in code, minification might even be worse), but add a whole new concept to reach that - and one that can't be used everywhere, so it will be confusing for people in a "what can I use in which context" way.

If you spend a few hours in the redux reactiflux channel, you'll see people struggling with all these concepts they have to learn at once, I'd rather not add another concept if it doesn't offer great value, and I can't see that value there.

@joelnet
Copy link

joelnet commented Jan 27, 2020

since api is passed in with getState and dispatch, maybe actions could be added to that? or as this.actions.actionFinished().

Honestly, I don't see the value of dispatch.slice.name() or something like that over dispatch(slice.actions.name()) which you could even get down to dispatch(actions.name()) or just dispatch(name()) when exporting/using a destructured slice.

In the best case, it may save a few characters (only in code, minification might even be worse), but add a whole new concept to reach that - and one that can't be used everywhere, so it will be confusing for people in a "what can I use in which context" way.

If you spend a few hours in the redux reactiflux channel, you'll see people struggling with all these concepts they have to learn at once, I'd rather not add another concept if it doesn't offer great value, and I can't see that value there.

I agree!

@phryneas
Copy link
Member Author

This is pretty much superseded by #547

@phryneas phryneas closed this Jun 13, 2020
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.

2 participants