Require type to be specified on actions
#564
Conversation
|
@acdlite @johanneslumpe @goatslacker What do you think? Should we do this? |
|
I like the requirement, the error message is probably confusing though if you want to let the dev know that they are not passing in the correct action. |
|
@goatslacker Can you tell me what about the message is confusing? |
|
It could introduce an unnecessary bar in the future when we might develop use cases for Reducers that don't rely on type. |
|
I could go either way on this. The only semi-reasonable situation I can think of where this might get in the way is when using non-plain-object actions, like Immutable.js Maps, in which case |
|
I feel that this would allow us to further standardize the actions we accept and their structure. @acdlite how feasible is using immutable for actions over plain objects? Would it be possible for those people to just tack on a This change definitely catches errors where one might destructure the wrong action type from a constants file and else would have a hard time noticing why things break (typos etc.). I' learning towards this. |
|
@johanneslumpe Couldn't that be implemented as middleware? If you want to throw errors if the action doesn't have a type, you could write a middleware that does that instead of putting in the core.
It's also already on @acdlite's FSA spec, which a lot of people seem to be using. |
|
@edge That spec is exactly why I think having it in the core is a good thing - it furthers the standardization of the action shape. It is definitely possible to write a middleware which does the checking, no arguing there. But why force the user to do this type of check on their own, when it can happily live in the core and provide a benefit for all? In terms of compile-to languages: I'd assume that you can put anything into a hash. If something would prevent you from adding a certain string as a key that would be kind of weird, no? |
Why? Even if reducers don't look at type (e.g. "entities" reducer in "real-world" example folder), the actions still need types for logging and cases when later you might want to add a reducer handling this specific type. |
We don't allow them anyway. Currently we force actions to be plain objects. |
The most common mistake we want to catch is typo in constant name (and thus undefined |
@gaearon Forgot about that. If we're already making that requirement, then it this additional requirement seems only natural. |
|
@gajus would have complaints, given his convention of using |
|
I agree with @edge that this belongs be in the middleware. @conundrumer Yes, but there is a way to use Flux Standard Action (i.e. "type" property) with Canonical Reducer Composition, https://github.com/gajus/canonical-reducer-composition#flux-standard-action. redux-convention is used to convert actions from FSA to CCA and vice-versa. In theory, if such a requirement were imposed, you could use the same middleware to workaround it, i.e. convert the action back to FSA at the end of the middleware store. On the other hard, I don't agree that "type" is the right name for the property to begin with. |
Maybe it wasn't, but since this is pretty much a convention at this point, there needs to be a very compelling reason to break everyone by changing the property name. However, some property name needs to be enforced because there is much debugging benefit to preventing “bad constant import” errors early. Therefore we converge on |
|
What would be the recommended type of API actions in the real-world example then? Since now it throws an error. |
|
What error? I will double check but examples have worked fine for a long period of time since this merge. The types are specified by the API middleware that interprets these actions. |
|
My bad, my API middleware wasn't plugged in 😓 |
Fixes #541