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

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

Closed
kswope opened this issue Aug 16, 2015 · 17 comments
Closed

Comments

@kswope
Copy link

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
Copy link
Contributor

merk commented Aug 16, 2015

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

@ghost
Copy link

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
#541 (comment).

@gaearon
Copy link
Contributor

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
Copy link
Author

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
Copy link
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

@wmertens
Copy link
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
#541 (comment).

Wout.
(typed on mobile, excuse terseness)

@gaearon
Copy link
Contributor

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
Copy link
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
#541 (comment).

Wout.
(typed on mobile, excuse terseness)

@gaearon
Copy link
Contributor

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
Copy link
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
#541 (comment).

Wout.
(typed on mobile, excuse terseness)

@gaearon
Copy link
Contributor

gaearon commented Sep 12, 2015

Done in 3.0.0.

@gaearon gaearon closed this as completed Sep 12, 2015
@frankoid
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

@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
Copy link
Contributor

gaearon commented Oct 1, 2015

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

@sktguha
Copy link
Contributor

sktguha commented Aug 25, 2020

why is type: null allowed as a valid action ? isn't it better to check for both null and undefined ? Are there any use cases where null is a valid action type ? maybe 0 is valid but don't think null is

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

7 participants