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

Middleware not dispatched through on init #465

Closed
phated opened this issue Aug 11, 2015 · 19 comments
Closed

Middleware not dispatched through on init #465

phated opened this issue Aug 11, 2015 · 19 comments

Comments

@phated
Copy link

phated commented Aug 11, 2015

I'm not sure if this has been discussed before (quick search got me no where), but I am noticing that no middleware is executed when redux does its init actions. Is this on purpose or just something that has come out of the functional composition nature of redux? I would like to use middleware to ensure payload objects on each of my actions, but the init actions cause things to explode due to not passing through middleware.

@acdlite
Copy link
Collaborator

acdlite commented Aug 12, 2015

Is this on purpose or just something that has come out of the functional composition nature of redux?

Both :) init action is a no-op action that exists solely to prime the store with an initial state, using default parameters with reducers:

function reducer(state = initialState, action) {...}

Since state is undefined at initialization,* and init is a no-op action, initialState is returned as... the initial state.

I would like to use middleware to ensure payload objects on each of my actions, but the init actions cause things to explode due to not passing through middleware.

Not sure I understand what the problem is. Could you elaborate?

*unless you pass an initial state to createStore(), in which case it works as expected

@acdlite
Copy link
Collaborator

acdlite commented Aug 12, 2015

For context, here's where the init action is dispatched: https://github.com/gaearon/redux/blob/breaking-changes-1.0/src/createStore.js#L140

@phated
Copy link
Author

phated commented Aug 12, 2015

The thing that lead me down this path was just a code-style preference in my reducers.

This is a poorly thought out example of the style I was looking to achieve.

function status(state = initial, { type, payload }){
    const { notification, isError } = payload;

    switch(type){
        case UPDATE_STATUS:
            return { notification, isError };
        default:
            return state;
    }  
}

As you can probably tell, moving the destructuring from payload to the top of the reducer keeps the switch statements much simpler/cleaner, especially as things start to grow, or assignment is being done, etc.

However, this pattern fails because on Redux's init, there is no payload. I definitely don't think Redux should add a payload property to the actions it dispatches for init, but I would like to put a middleware in front of my reducers that guarantees a payload property that is an object. This still falls apart because the init action isn't passed through middleware.

I understand that the init action is a no-op, but middleware is supposed to act upon actions (whether they are no-ops or not), so it seems strange that middleware is skipped for initialization.

Another thing that I would probably want in the future is to log the init actions to know everything is booting up correctly throughout the system. This could be logged by logging middleware if init was passed through the middleware stack.

I understand the example I gave that got me to this place could be solved by using a default for the payload property of my destructure, but hopefully we can look at this from a high-level perspective and not get bogged down in my simple example.

@acdlite
Copy link
Collaborator

acdlite commented Aug 12, 2015

Thanks for the additional info. I understand now.

I understand that the init action is a no-op, but middleware is supposed to act upon actions (whether they are no-ops or not), so it seems strange that middleware is skipped for initialization.

The init action is unique because it's the only dispatch that is called by Redux itself, rather than by the developer (or some extension added by the developer). You were correct in your original comment in saying that this has to do with the functional compositional nature of Redux. When you do a normal dispatch with middleware, you're calling it on a store interface that has been set up like so:

const store = applyMiddleware(m1, m2, m3)(createStore)(reducer, initialState);

But since the init action is dispatched by the base createStore() function, it has no awareness of middleware. It's as if the store was created like this:

const store = createStore(reducer, initialState);

In a practical sense, this should only a problem if reducers are making hard assumptions about the shape of actions, which probably isn't a good idea.

Theoretically, though, it may make more sense to get rid of the init action, and require the use to either 1) pass an initial state to createStore(), or 2) dispatch an init action themselves, with whatever kind/type/shape of action they desire.

@gaearon
Copy link
Contributor

gaearon commented Aug 12, 2015

Might be related to #350, #376

@acstll
Copy link

acstll commented Aug 13, 2015

@phated even if the init action did pass through middleware and had the missing payload object attached to it, wouldn't its properties (in the example) { notification, isError } still be undefined?

I agree with this:

I understand that the init action is a no-op, but middleware is supposed to act upon actions (whether they are no-ops or not), so it seems strange that middleware is skipped for initialization.

Maybe the question is: is it OK to handle the init action in middleware, knowing it shouldn't be handled or referenced in reducers?

Re: #376 and according to @acdlite comment above, the only way for the init action to go through middleware would be to have these applied after creating the store and not before, something like this:

const store = createStore(reducer, initialState)
applyMiddleware(store, m1, m2, m3)

Is this correct?

@phated
Copy link
Author

phated commented Aug 13, 2015

@acstll it is okay that they are undefined, because they are only used when the action type exists. However, if payload is undefined, an error is thrown because you can't destructure from undefined.

You are correct that the init action is not supposed to be explicitly acted on, but proper middleware doesn't act on the action type, they usually just enhance functionality (at worst, based on action shape)

I am trying to think how the invariant logic would deal with this versus the initial dispatch in createStore. I am wondering if it can be done as a store enhancer, like the middleware.

@acstll
Copy link

acstll commented Aug 13, 2015

@phated

it is okay that they are undefined, because they are only used when the action type exists.

that is so true 😄

@johanneslumpe
Copy link
Contributor

@phated Not sure if you already thought about this, but you could do something like this:

(state, { type, payload = {} }) => {
  const { notification, isError } = payload;
  /* ... */
}

This way nothing has to change and it will work just fine with the init action because if payload is undefined it will be set to the default object.

@phated
Copy link
Author

phated commented Aug 13, 2015

@johanneslumpe that was noted as the very last sentence of my issue. It isn't about the example that lead me here, it is about the overarching problem.

@johanneslumpe
Copy link
Contributor

Oh sorry @phated I missed that part. Yeah I understand what you are trying to address with this issue, wanted to offer an interim solution - but you already had that one going anyway ;)

@acstll
Copy link

acstll commented Aug 14, 2015

the only way for the init action to go through middleware would be to have these applied after creating the store and not before… Is this correct?

To answer my own question: no, that's not correct.

The only way to get that init action to pass through middleware is to have no default init action. Namely what @acdlite proposed here:

Theoretically, though, it may make more sense to get rid of the init action, and require the use to either 1) pass an initial state to createStore(), or 2) dispatch an init action themselves, with whatever kind/type/shape of action they desire.

I'm +1 on this.

If you need that @@redux/INIT for hot reloading, you can dispatch it manually inside the React component which handles hot reloading, right?

@gaearon
Copy link
Contributor

gaearon commented Sep 24, 2015

I don't currently think we're going to do this. There don't seem to be enough upsides, and the minor inconveniences don't seem to me like they're worth changing an API to force the user to dispatch something like INIT themselves.

If you'd like to continue this discussion please do this in form of a PR (both to source and examples) so we have a specific proposition to talk about. I still think it's unlikely to get merged, but IMO we've ran out of depth discussing it here, and without the code, there isn't much to talk about.

@gaearon gaearon closed this as completed Sep 24, 2015
@gaearon gaearon reopened this Apr 2, 2016
@gaearon gaearon added the bug label Apr 2, 2016
@gaearon
Copy link
Contributor

gaearon commented Apr 2, 2016

Meh. The inconsistency of dispatching @@INIT too early to reducers keeps coming up again and again:

We should probably fix it here, even at the cost of a breaking change. Some ideas:

  • Delay @@INIT until the first getState() and do it lazily? (side effects 😮 )
  • I’m out of ideas but let me know if you have any

I still wouldn’t want people to put .dispatch({}) after their createStore() calls, this looks silly.

@tomkis
Copy link
Contributor

tomkis commented Apr 3, 2016

The more I think about it, the more I am inclined to a variant that people should provide their own bootstrap action, because even if I managed to implement deferred initialization after enhancer is applied, it still does not guarantee that this is the first dispatched action, because for example redux-devtools has its own init function which is dispatched before Redux init action (due to an order of enhancers being applied).

Concept of Enhancers unfortunately does not provide any guarantees. Therefore for me, as a library author it does make sense to rely on custom bootstrapping action. Maybe we should advocate this in docs?

Speaking about #1240 - this is not directly related because people are trying to solve something which clearly can live in userland (#1240 (comment))

@tappleby
Copy link
Contributor

tappleby commented Apr 4, 2016

Im not a huge fan of using the lazily dispatched @@INIT action.

Could we take advantage of the fact that store enhancers are now a first class concept (#1294) and ensure the store is initialized w/ the enhancer before the init dispatch action is called?

The above change would probably involve deprecating the old method of applying store enhancers.

@felipeochoa
Copy link

Sorry if this is the wrong issue to comment on (hard to keep track with all the inter-dependencies) but here's another vote and use-case for having middleware see the @@INIT action:

I have a middleware that opens up a WebSocket session, where the sub-protocol allows connecting and disconnecting without dropping the socket. There's some store-dependent logic about whether to clear the session, disconnect codes, etc. that run after next(action) but also on startup (since session information may be preloaded).:

function middleware(store) {
  const session = new StatefulSocketConnection(...);
  processStateChange(session, undefined, state);
  return next => action => {
    // Pulling out the entire state is unnecessary, but makes processStateChange feel cleaner
    const prevState = store.getState();
    const ret = next(action);
    const state = store.getState();
    processStateChange(session, prevState, state);
    return ret;
  }
}

Splitting out processStateChange feels like a workaround to avoid code duplication, since it is the meat of the middleware and would comfortably live in-line after next(action).

@lilasquared
Copy link

Here's something interesting that I noticed. If you add this test:

it('dispatches an initial action to reducers', () => {
  const spy = jest.fn()
  const store = createStore((state, action) => spy())
  expect(spy.mock.calls.length).toBe(1)
})

It makes sense that it passes. (not sure if its needed, but it is a passing test)
However modifying the test just a little bit makes it clear that the initial action dispatch can result in some unexpected behavior

it('dispatches an initial action to reducers', () => {
  const store = createStore((state = 0, action) => state + 1, 1)
  expect(store.getState()).toEqual(1)
})

This test fails because of the init action and the fact that this reducer doesn't care what action is passed to it. I'm not sure what the expected behavior would be here because the reducer is so simple to begin with , maybe it's not relevant at all. But it feels like this test should pass.

@timdorr
Copy link
Member

timdorr commented Oct 23, 2017

We have a warning in place now to prevent this problem at all: #1485

@timdorr timdorr closed this as completed Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants