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

suggestion, don't allow store to dispatch action.type of undefined or null #541

Closed
kswope opened this Issue Aug 16, 2015 · 16 comments

Comments

6 participants
@kswope

kswope commented Aug 16, 2015

I've haven't given this a lot of deep though so I'll probably someday want this post erased from the internet, but a few times already I've screwed up my 'constants',

You can see in the below example, if I misspell ACCRUE either here or in the constants file (I'm not even sure if its spelled correctly here), it becomes undefined and then the reducer happily uses the default switch, and because store.dispatch is inclined to put whatever it pleases (@@init/etc) through the default path, I can't use that to catch screw ups. Why not throw an exception when a falsey action.type is dispatched, since its so easy to do and sometimes difficult to catch, even in tests.

     switch ( action.type ) {                                                                                                                        
       case constants.AUTH_TOKEN:                                                                                                                    
         return authenticate(state, action)                                                                                                          
       case constants.ACCRUE:                                                                                                                        
         return accrue(state, action)                                                                                                                
       default:                                                                                                                                      
         return state                                                                                                                                
     }   

@merk

This comment has been minimized.

Show comment
Hide comment
@merk

merk Aug 16, 2015

Contributor

You can define your own middleware that would behave this way.

Contributor

merk commented Aug 16, 2015

You can define your own middleware that would behave this way.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 16, 2015

Are there any cases where undefined would a valid action type? This
suggestion would help avoid a pretty annoying problem that is really fairly
trivial, but very easy to overlook, resulting in wasted time. The reason I
ask if there are cases where undefined would be valid is that I'd hate to
see use cases blocked by unnecessary constraints, but if there aren't
any/many then making this the default would be better than putting it in
middleware (which people probably won't remember to add until after they've
pulled out their hair). If not an error, at least a warning?
On Sun, Aug 16, 2015 at 1:20 AM Tim Nagel notifications@github.com wrote:

You can define your own middleware that would behave this way.


Reply to this email directly or view it on GitHub
https://github.com/rackt/redux/issues/541#issuecomment-131491620.

ghost commented Aug 16, 2015

Are there any cases where undefined would a valid action type? This
suggestion would help avoid a pretty annoying problem that is really fairly
trivial, but very easy to overlook, resulting in wasted time. The reason I
ask if there are cases where undefined would be valid is that I'd hate to
see use cases blocked by unnecessary constraints, but if there aren't
any/many then making this the default would be better than putting it in
middleware (which people probably won't remember to add until after they've
pulled out their hair). If not an error, at least a warning?
On Sun, Aug 16, 2015 at 1:20 AM Tim Nagel notifications@github.com wrote:

You can define your own middleware that would behave this way.


Reply to this email directly or view it on GitHub
https://github.com/rackt/redux/issues/541#issuecomment-131491620.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 16, 2015

Collaborator

I agree it's something we should do by default. This would also effectively force everyone to use "type" instead of bikeshedding on the property name, which is good for the ecosystem. Anybody with use cases for empty type?

Collaborator

gaearon commented Aug 16, 2015

I agree it's something we should do by default. This would also effectively force everyone to use "type" instead of bikeshedding on the property name, which is good for the ecosystem. Anybody with use cases for empty type?

@kswope

This comment has been minimized.

Show comment
Hide comment
@kswope

kswope Aug 16, 2015

Looking at this semantically, is an action.type of undefined or null even worthy of the term "action"?

At its worst being opinionated about falsey actions could discourage some smelly code.

kswope commented Aug 16, 2015

Looking at this semantically, is an action.type of undefined or null even worthy of the term "action"?

At its worst being opinionated about falsey actions could discourage some smelly code.

@somebody32

This comment has been minimized.

Show comment
Hide comment
@somebody32

somebody32 Aug 16, 2015

Contributor

But what if there is some logic inside the action creator that decides that there is no need to proceed with dispatching (just think about any conditional logic that uses getState)?
Now you can "abort" dispatching, just by returning {} from the action creator (not really abort, just reducers will ignore this action entirely).
But if the action type will be required, workaround will be way more verbose: {type: PLEASE_IGNORE_ME } + having PLEASE_IGNORE_ME in the constants file

Contributor

somebody32 commented Aug 16, 2015

But what if there is some logic inside the action creator that decides that there is no need to proceed with dispatching (just think about any conditional logic that uses getState)?
Now you can "abort" dispatching, just by returning {} from the action creator (not really abort, just reducers will ignore this action entirely).
But if the action type will be required, workaround will be way more verbose: {type: PLEASE_IGNORE_ME } + having PLEASE_IGNORE_ME in the constants file

@wmertens

This comment has been minimized.

Show comment
Hide comment
@wmertens

wmertens Aug 16, 2015

Contributor

But then you need to thunk, whereas a plain action creator can't just
return e.g. false right now.

On Sun, Aug 16, 2015, 11:37 Johannes Lumpe notifications@github.com wrote:

@somebody32 https://github.com/somebody32 that action creator could
then just not call dispatch?


Reply to this email directly or view it on GitHub
https://github.com/rackt/redux/issues/541#issuecomment-131512659.

Wout.
(typed on mobile, excuse terseness)

Contributor

wmertens commented Aug 16, 2015

But then you need to thunk, whereas a plain action creator can't just
return e.g. false right now.

On Sun, Aug 16, 2015, 11:37 Johannes Lumpe notifications@github.com wrote:

@somebody32 https://github.com/somebody32 that action creator could
then just not call dispatch?


Reply to this email directly or view it on GitHub
https://github.com/rackt/redux/issues/541#issuecomment-131512659.

Wout.
(typed on mobile, excuse terseness)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 17, 2015

Collaborator

But what if there is some logic inside the action creator that decides that there is no need to proceed with dispatching (just think about any conditional logic that uses getState)?

As @wmertens notes, you can do that just fine with redux-thunk. It doesn't force you to call dispatch(). You can return; from the returned function, and it will just work.

Collaborator

gaearon commented Aug 17, 2015

But what if there is some logic inside the action creator that decides that there is no need to proceed with dispatching (just think about any conditional logic that uses getState)?

As @wmertens notes, you can do that just fine with redux-thunk. It doesn't force you to call dispatch(). You can return; from the returned function, and it will just work.

@wmertens

This comment has been minimized.

Show comment
Hide comment
@wmertens

wmertens Aug 17, 2015

Contributor

Still, I kinda feel like a plain action creator should be allowed to return
false (exactly that, nothing else) to not dispatch anything, much like
React allows to not create an element when you return null or false.

Obviously, that's trivial middleware, but still…

On Mon, Aug 17, 2015, 17:43 Dan Abramov notifications@github.com wrote:

But what if there is some logic inside the action creator that decides
that there is no need to proceed with dispatching (just think about any
conditional logic that uses getState)?

As @wmertens https://github.com/wmertens notes, you can do that just
fine with redux-thunk. It doesn't force you to call dispatch(). You can
return; from the returned function, and it will just work.


Reply to this email directly or view it on GitHub
https://github.com/rackt/redux/issues/541#issuecomment-131867753.

Wout.
(typed on mobile, excuse terseness)

Contributor

wmertens commented Aug 17, 2015

Still, I kinda feel like a plain action creator should be allowed to return
false (exactly that, nothing else) to not dispatch anything, much like
React allows to not create an element when you return null or false.

Obviously, that's trivial middleware, but still…

On Mon, Aug 17, 2015, 17:43 Dan Abramov notifications@github.com wrote:

But what if there is some logic inside the action creator that decides
that there is no need to proceed with dispatching (just think about any
conditional logic that uses getState)?

As @wmertens https://github.com/wmertens notes, you can do that just
fine with redux-thunk. It doesn't force you to call dispatch(). You can
return; from the returned function, and it will just work.


Reply to this email directly or view it on GitHub
https://github.com/rackt/redux/issues/541#issuecomment-131867753.

Wout.
(typed on mobile, excuse terseness)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 17, 2015

Collaborator

Still, I kinda feel like a plain action creator should be allowed to return
false (exactly that, nothing else) to not dispatch anything, much like
React allows to not create an element when you return null or false.

I don't agree. Any real use case I have in mind involves getState() which a plain action creator doesn't have access to anyway.

Collaborator

gaearon commented Aug 17, 2015

Still, I kinda feel like a plain action creator should be allowed to return
false (exactly that, nothing else) to not dispatch anything, much like
React allows to not create an element when you return null or false.

I don't agree. Any real use case I have in mind involves getState() which a plain action creator doesn't have access to anyway.

@wmertens

This comment has been minimized.

Show comment
Hide comment
@wmertens

wmertens Aug 17, 2015

Contributor

D'oh you're absolutely right...
and of course actioncreators could also send an action of type "ERROR" or
whatever.

On Mon, Aug 17, 2015 at 9:04 PM Dan Abramov notifications@github.com
wrote:

Still, I kinda feel like a plain action creator should be allowed to return
false (exactly that, nothing else) to not dispatch anything, much like
React allows to not create an element when you return null or false.

I don't agree. Any real use case I have in mind involves getState() which
a plain action creator doesn't have access to anyway.


Reply to this email directly or view it on GitHub
https://github.com/rackt/redux/issues/541#issuecomment-131931404.

Wout.
(typed on mobile, excuse terseness)

Contributor

wmertens commented Aug 17, 2015

D'oh you're absolutely right...
and of course actioncreators could also send an action of type "ERROR" or
whatever.

On Mon, Aug 17, 2015 at 9:04 PM Dan Abramov notifications@github.com
wrote:

Still, I kinda feel like a plain action creator should be allowed to return
false (exactly that, nothing else) to not dispatch anything, much like
React allows to not create an element when you return null or false.

I don't agree. Any real use case I have in mind involves getState() which
a plain action creator doesn't have access to anyway.


Reply to this email directly or view it on GitHub
https://github.com/rackt/redux/issues/541#issuecomment-131931404.

Wout.
(typed on mobile, excuse terseness)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 12, 2015

Collaborator

Done in 3.0.0.

Collaborator

gaearon commented Sep 12, 2015

Done in 3.0.0.

@gaearon gaearon closed this Sep 12, 2015

@frankoid

This comment has been minimized.

Show comment
Hide comment
@frankoid

frankoid Sep 29, 2015

Is this change compatible with redux-thunk? I'm trying to upgrade https://github.com/frankoid/flux-comparison to redux 3, but when I upgrade from 2.0.0 to 3.0.2 then I start seeing an Actions may not have an undefined "type" property. Have you misspelled a constant? error.

frankoid commented Sep 29, 2015

Is this change compatible with redux-thunk? I'm trying to upgrade https://github.com/frankoid/flux-comparison to redux 3, but when I upgrade from 2.0.0 to 3.0.2 then I start seeing an Actions may not have an undefined "type" property. Have you misspelled a constant? error.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 29, 2015

Collaborator

It's compatible. We're actually doing a good thing in 3.0: this line uses a constant that was never defined.

Collaborator

gaearon commented Sep 29, 2015

It's compatible. We're actually doing a good thing in 3.0: this line uses a constant that was never defined.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 29, 2015

Collaborator

@frankoid By the way, if you don't mind, I'd like taking a stab at updating it myself. There are a few things I'd change there..

Collaborator

gaearon commented Sep 29, 2015

@frankoid By the way, if you don't mind, I'd like taking a stab at updating it myself. There are a few things I'd change there..

@frankoid

This comment has been minimized.

Show comment
Hide comment
@frankoid

frankoid Sep 30, 2015

@gaearon thanks! I've got it working now. Happy for you to update it yourself - I am learning React and Redux and was using the upgrade as a learning exercise, so I'm not up to speed with Redux best practices.

frankoid commented Sep 30, 2015

@gaearon thanks! I've got it working now. Happy for you to update it yourself - I am learning React and Redux and was using the upgrade as a learning exercise, so I'm not up to speed with Redux best practices.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 1, 2015

Collaborator

@frankoid FYI, I updated it in the flux-comparison repo.

Collaborator

gaearon commented Oct 1, 2015

@frankoid FYI, I updated it in the flux-comparison repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment