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

Add basic middleware api to default dispatcher #63

Merged
merged 4 commits into from Jun 10, 2015

Conversation

@acdlite
Copy link
Collaborator

acdlite commented Jun 9, 2015

This gives users the ability to pass global "action middleware" (terminology still in flux) to the default dispatcher.

// Before
createRedux(stores, initialState);
// After
createRedux(stores, initialState, middleware);

What is middleware?

Middleware is a function that wraps the dispatch() method, or another middleware. For example, to use a middleware:

// Instead of this
dispatch(action)
// do this
middleware(dispatch)(action)

Multiple middleware can be composed manually

middleware1(middleware2(dispatch))(action)

Or using the provided compose() utility:

// All are equivalent:
middleware1(middleware2(dispatch))(action)
compose(middleware1, middleware2)(dispatch)(action)
compose(middleware1, middleware2, dispatch)(action)

The compose() middleware may seem trivial, but it makes it allows you to easily compose an array of middleware using spread notation:

compose(...middlewares);

Because middleware simply wraps dispatch() to return a function of the same signature, they can be used completely within userland. However, for the most part, you'll want to apply them globally to every action dispatch.

Example of how to write middleware

Here's a middleware for adding naive promise support to Redux:

function promiseMiddleware(next) {
  return action =>
    action && typeof action.then === 'function'
      ? action.then(next)
      : next(action);
}

Use cases

Usually, they'll be used like schedulers. They can be used to implement promise support (a la Flummox), observable support, generator support, whatever. Or they can simply be used for side-effects like logging.

How this affects the core API

This PR does not break the default behavior of the existing API. For instance, while perform() has been re-implemented as middleware internally, it is the default middleware if none is configured by the user.

In the future, Redux may provide some additional middlewares for things like optimistic updates, but they will be completely optional and not enabled by default.

Why is middleware special? Why not simply tell users to provide a custom Dispatcher and call it a day?

Custom dispatchers are the way to go for advanced functionality like time travel and transactions. But middleware is special because of a very important property: because middleware wraps the dispatch() method, it's inherently compatible with any dispatcher, regardless of implementation.

Still, middleware is totally optional, and each dispatcher implementation can choose whether or not to support them. (At the global level, that is — middleware can always be used from the calling site.)

@@ -5,7 +5,7 @@ export default class Redux {
constructor(dispatcher, initialState) {
if (typeof dispatcher === 'object') {
// A shortcut notation to use the default dispatcher
dispatcher = createDispatcher(composeStores(dispatcher));
dispatcher = createDispatcher(composeStores(dispatcher), initialState);

This comment has been minimized.

Copy link
@emmenko

emmenko Jun 9, 2015

Contributor

I'm wondering why you pass initialState as second argument but in the createDispatcher definition you expect a middleware function?

This comment has been minimized.

Copy link
@acdlite

acdlite Jun 9, 2015

Author Collaborator

@emmenko It's because we're overloading the signature of the constructor. initialState is actually middleware, just like dispatcher in that call is actually a stores hash. You make a good point, though... it should be the third argument so we can still set the initialState. Eventually we'll need to rethink this (perhaps by using an options object instead) but for now we're trying not to break the existing API.

This comment has been minimized.

Copy link
@emmenko

emmenko Jun 9, 2015

Contributor

Yeah ok I guessed that. And probably an options object would be better yes.

PS: we are already breaking the API ;) Better do it now then later when people already start using it :)

This comment has been minimized.

Copy link
@acdlite

acdlite Jun 9, 2015

Author Collaborator

Haha it'll change soon once we get to a stabler place, but in the meantime we don't want to be breaking the API for every new PR.

This comment has been minimized.

Copy link
@emmenko

emmenko Jun 9, 2015

Contributor

Yep 👍

@acdlite acdlite referenced this pull request Jun 9, 2015
@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 9, 2015

Does the default middleware need to be special? Can we let any middleware read state?

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

IMO, If a middleware needs to read the current state, it should probably be implemented inside the dispatcher instead. Action middleware is just about transforming a stream of actions, nothing more. They should know nothing about the current state, unless of course it's passed in at the calling site.

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

Also I'm not a fan of how the current dispatch() method lets a callback read state, but I kept that since it's part of the existing API.

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 9, 2015

Also I'm not a fan of how the current dispatch() method lets a callback read state, but I kept that since it's part of the existing API.

If something isn't good, we should change it! But I think reading state in action creators is important. Putting a Redux instance into a separate module and calling getState() can be harder for the beginners.

There's definitely a problem with the current API though. The state will become stale. If you're doing an await call inside an action creator, it's likely the state is already stale by the time it ends. Therefore it's a bad API.

Here's what I propose:

  • Change createDispatcher to have (absolutely) no middleware by default.
  • Change Redux constructor to not accept the middleware. (It's confusing that middleware parameter is only used in the “convenience” version. I think it's fair that, if people want custom middleware, they should create dispatcher explicitly and pass it there instead.)
  • Move the default middleware inside Redux as a private instance method and pass it to createDispatcher when using the “convenience” shortcut API where Redux calls createDispatcher. Because the default middleware is an instance method, it has the access to this.getState, so we don't need to somehow make this middleware special.
  • Instead of passing state as the second parameter, pass getState so we avoid staleness.

Thoughts?

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

But I think reading state in action creators is important. Putting a Redux instance into a separate module and calling getState() can be harder for the beginners.

Can you give me an example of when it's a good idea for action creators to read directly from the store? It sounds like a foot gun to me.

If the call site has access to the dispatch method, then it also has access to the current state, and can pass whatever values it needs to the action creator.

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

Also, on a separate subject, staleness isn't an issue in the current version because the state that is passed to next references a closed over value.

Edit: Never mind, you're right.

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 9, 2015

Can you give me an example of when it's a good idea for action creators to read directly from the store? >It sounds like a foot gun to me.

What's footgunny about it?
Here's an example of how I use that:

export function requestStarredReposPage(login, isInitialRequest) {
  // Exit early if already fetching, or if there is nothing to fetch.
  if (StarredReposByUserStore.isExpectingPage(login) ||
      StarredReposByUserStore.isLastPage(login)) {
    return;
  }

  // Ignore first page request when component is mounting if we already
  // loaded at least one page before. This gives us instant Back button.
  if (isInitialRequest && StarredReposByUserStore.getPageCount(login) > 0) {
    return;
  }

  const nextPageUrl = StarredReposByUserStore.getNextPageUrl(login);
  dispatchAsync(RepoAPI.getStarredReposPage(login, nextPageUrl), {
    request: ActionTypes.REQUEST_STARRED_REPOS_PAGE,
    success: ActionTypes.REQUEST_STARRED_REPOS_PAGE_SUCCESS,
    failure: ActionTypes.REQUEST_STARRED_REPOS_PAGE_ERROR
  }, { login });
}

If the call site has access to the dispatch method, then it also has access to the current state, and can pass whatever values it needs to the action creator.

That's not strictly true. The callsite is likely to be a React component. We currently don't provide a convenient way to read arbitrary state from a React component. You could use Connector with a super broad select but that might become a perf issue. There are many cases when you want to read the state, but not be subscribed to it, and it's usually convenient to do inside action creator, because such “state reads” are usually performed for pagination, figuring out if the entity is already cached, etc—something that isn't really tied to a particular component, but to a particular action instead.

Also, on a separate subject, staleness isn't an issue in the current version because the state that is passed to next references a closed over value.

But once it's passed to a function it doesn't matter right? I'm thinking of the case when one action creator awaits something and by the time await is done, its state is stale. That it was closed over at the call site doesn't help here, I think..

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

Which part of your example couldn't be implemented by passing the state corresponding to the starred repo directly to the action creator?

You could use Connector with a super broad select but that might become a perf issue.

Why does it need to be super broad? The call site can select only the subset it needs to perform the action, which in many cases should overlap with the subset it needs to render the view. But in any case, we probably should be providing an on-demand getState() API to the component that would allow them to query the store on-demand without subscribing to changes.

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

And yeah you're write about the state staleness, my bad.

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 9, 2015

Why does it need to be super broad? The call site can select only the subset it needs to perform the action, which in many cases should overlap with the subset it needs to render the view.

It's still cumbersome to duplicate the same “selection and early exit” code across the components, especially if the same data is being selected, but the prop shape is already slightly different in the components.

I think it's good that one can specify that an action under some conditions doesn't make sense and shouldn't ever be called, in the action itself. This helps prevent incorrect action creator usage in a larger app.

But in any case, we probably should be providing an on-demand getState() API to the component that would allow them to query the store on-demand without subscribing to changes.

Yeah, perhaps.

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

It's still cumbersome to duplicate the same “selection and early exit” code across the components, especially if the same data is being selected,

Isn't that what helper functions are for? :) We already need to replace those store accessor methods that people are so fond of.

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

I think it's good that one can specify that an action under some conditions doesn't make sense and shouldn't ever be called, in the action itself. This helps prevent incorrect action creator usage in a larger app.

You can still do that, you just have to pass the state in rather than accessing directly from the action creator. A getState() API for the component solves this.

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 9, 2015

If you're invoking an action creator from another action creator, or somewhere from server code, you need to remember to pass it. Sure, it's doable, but isn't it just on the same level of convenience as having a dispatch function given to you?

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

Maybe we can solve this with higher-level middleware:

createDispatcher({
  store,
  middleware: getState => compose(performMiddleware(getState), callbackMiddleware)
})
@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 9, 2015

What do you think about my suggestion in #63 (comment)? Keep it a normal middleware, but injected from Redux instance. This neatly sidesteps the problem because it already has access to the instance.

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

Yeah I like that solution for the default middleware.

What about custom middleware passed to createDispatcher()? If we let the default middleware access getState() then custom middleware should have that option, too. I think a higher-order function like above solves this elegantly.

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 9, 2015

OK that's fair. Let's give any middleware getState.

I don't like the additional “one level deeper” thing though. It feels more complex that it could have been. Why not change the middleware signature from (next) => (action) => () to (next, getState) => (action) => ()? composeMiddleware could still figure out how to compose that correctly.

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

Because then every middleware has to pass getState() to the next one. That's a larger burden to me, because most middleware will not need getState(). But, we could solve on the API level by checking the type of middleware passed to the constructor. If it's an array, then compose them like normal. If it's a function, then use the higher-order function.

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

Also what if later down the road, we decide that we want to change the middleware signature once again? Then everybody in userland has to update their middleware. Whereas if we stick with the most basic signature possible, it's future proof, because the only thing that needs to change is the higher-order function. It's even Flux library proof, because middleware as I've described it will work with any dispatch method, not just Redux!

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

(next) => (action) => () to (next, getState) => (action) => ()

Actually, it would be (next) => (action, getState) => ()

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 9, 2015

You convinced me, can you update the PR please?

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

Sure thing, I get to it ASAP.

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

What was your opinion about type-checking the middleware passed to createDispatcher()? #63 (comment) Yea or nay?

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 9, 2015

I don't mind built-in array composition if I see a nice use case for it.

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

I would leave it out for now and just make people specify a single middleware, but since the higher-order function and middleware are both functions, there's no way to use type checking to distinguish them.

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 9, 2015

Right. OK, let's allow only [] and (getState) => [] then.

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 9, 2015

👍

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 10, 2015

@gaearon Updated the PR according to our discussion.

@acdlite acdlite referenced this pull request Jun 10, 2015

return recurse;
}

This comment has been minimized.

Copy link
@acdlite

acdlite Jun 10, 2015

Author Collaborator

Hmm, on second thought this really should be its own module, implemented using higher-order middleware.

@acdlite

This comment has been minimized.

Copy link
Collaborator Author

acdlite commented Jun 10, 2015

I separated the default middleware to its own module. Now it's implemented just like any other middleware.

// A shortcut notation to use the default dispatcher
dispatcher = createDispatcher(
  composeStores(dispatcher),
  getState => [ thunkMiddleware(getState) ]
);

@gaearon What do you think of the name "thunk middleware"? It's more specific than "callback middleware," but I doubt many people are familiar with this term. I only know it because of co.

Edit: To clarify, it's still the default middleware... I merely changed the implementation.

@emmenko

This comment has been minimized.

Copy link
Contributor

emmenko commented on src/middleware/thunk.js in 763c148 Jun 10, 2015

I think generally we should write some JSDoc, at least for such pure functions

This comment has been minimized.

Copy link
Collaborator Author

acdlite replied Jun 10, 2015

Agreed

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 10, 2015

“Thunk” makes sense to me. Since you don't have to specify it by default, I think this name fits well.

dispatcher = createDispatcher(composeStores(dispatcher));
dispatcher = createDispatcher(
composeStores(dispatcher),
getState => [ thunkMiddleware(getState) ]

This comment has been minimized.

Copy link
@gaearon

gaearon Jun 10, 2015

Contributor

Nitpick: I'd prefer no spaces inside arrays

gaearon added a commit that referenced this pull request Jun 10, 2015
Add basic middleware api to default dispatcher
@gaearon gaearon merged commit 6c73f6f into master Jun 10, 2015
@gaearon gaearon deleted the middleware-api branch Jun 10, 2015
@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 10, 2015

👍

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 10, 2015

Let's update the README and do a release after RN support lands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.