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

Action validator throws on an action without a 'name' property (like the ones dispatched by the 'redux-devtools') #3

Closed
sompylasar opened this issue Aug 16, 2015 · 10 comments

Comments

@sompylasar
Copy link

The "Canonical Reducer Composition" validator checks every action object and throws if a name property is missing. But redux uses another convention – it uses type, not name, so no action object has a proper name in it by default.

The core @@redux/INIT action is worked around and ignored in redux-immutable, but the redux-devtools use another action called @@INIT which isn't.

I'm not sure if this is an issue of redux-immutable or the redux itself, but hopefully you guys will figure it out. CC @gajus @gaearon

For now I'm willing to uninstall redux-immutable rather than redux-devtools (until the issue is fixed somehow).

@gaearon
Copy link
Contributor

gaearon commented Aug 16, 2015

I don't think it's a Redux issue. "Type" is an established convention, at least in Flux world, that's where most of our conventions come from. There need to be very good reasons to break these conventions, and more so after we shipped 1.0 and lots of code is already written using "type". I think that's something for redux-immutable to reconsider.

@gajus
Copy link
Owner

gajus commented Aug 16, 2015

I have rewritten the test to check if action has type property and wether its value begins with "@@". These actions will be ignored, 85852fe.

@gaearon Is right in a sense that:

"Type" is an established convention, at least in Flux world, that's where most of our conventions come from.

This is also a convention defined in "flux-standard-action" spec. However, I have argued that "type" name as a convention does not make semantic sense.

"redux-immutable" implements the https://github.com/gajus/canonical-reducer-composition/ spec. Read the action object spec for more details. I will update the main "canonical-reducer-composition" description to include explanation why to use this "unconventional" spec over what has been established in flux land, but the short answer is: "canonical-reducer-composition" tries to be

  • semantically correct
  • follow conventions defined in the broader programming ecosystem
  • more strict (e.g. FSA defines 3 allowable name variations of the "name" property: "type", "actionType" or "actionId" all of which are derived from the legacy standard and only the latter making the most sense.)

In the mean time, I am working on dev-tools that will work with both "redux" and "redux-immutable".

@asaf
Copy link
Contributor

asaf commented Sep 2, 2015

@gajus This also breaks the logger middleware (it shows 'undefined' instead of the action type)
The community grow, middleware is the most pluggable way to extends Redux and is all about intercepting actions, if you don't get aligned with Redux convention things will just break,

At least make it configurable where default could be 'name',

(I guess we can just give the combineReducers a 2nd hash options where you can configure the property name that indicates the action identifier.)

Thanks.

@gajus
Copy link
Owner

gajus commented Sep 2, 2015

@asaf Just an idea, but why not have a function at the top of the middleware chain that maps "type" to "name" in every action it receives? This way anyone can use whatever name they choose if they don't like "name" convention. I can write a quick middleware package for that.

The same middleware function could map other properties that do not meet the standard, such as "payload".

Does that sound like a good solution?

@gajus
Copy link
Owner

gajus commented Sep 2, 2015

A simplified version would look like this:

store = applyMiddleware(
    mapper.fromCCAtoFSA,
    /* all middleware that needs to be FSA compatible */
    logger,
    mapper.fromFSAtoCCA
)(createStore)(reducer, state);

Where the mapper is something along the lines of:

let fromCCAtoFSA,
    fromFSAtoCCA;

fromCCAtoFSA = (store) => (next) => (action) => {
    next({
        type: action.name,
        payload: action.data
    });
};

fromFSAtoCCA = (store) => (next) => (action) => {
    next({
        name: action.type,
        data: action.payload
    });
};

export default {
    fromCCAtoFSA,
    fromFSAtoCCA
};

@sompylasar
Copy link
Author

@gajus 👍 I like this convention-conversion stuff.

@gajus
Copy link
Owner

gajus commented Sep 2, 2015

Ok. Will research certain aspects of the conversion (such as Error mapping) and release a package. Should be before the end of the day.

@gajus
Copy link
Owner

gajus commented Sep 2, 2015

@gajus gajus closed this as completed Sep 2, 2015
@asaf
Copy link
Contributor

asaf commented Sep 4, 2015

@gajus This is still problematic, as @gaearon mentioned, "lots of code is already written using "type"", this also means reducers are being written in the community and expecting type, conversion to 'name' won't satisfy,

IMO, If you aim CCA out of the redux community you can diverge, but if you aim for Redux, you should be compatible with its standards otherwise community code will break.

@gajus
Copy link
Owner

gajus commented Sep 4, 2015

I use CRC as well as CCA in all the projects that I develop. I am sharing the CRC spec. I am sharing some libraries with the open source world and provide a method of bridging the different conventions. Sounds fair to me.

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

4 participants