Use redux constants and action creator functions

[jeffcarbs]

First I just to want to say thanks for publishing this library! It feels really well thought-out and it's been easy to use.

---

Explain the motivation for making this change. What existing problem does the pull request solve?

The way redux actions are currently implemented in the codebase is brittle and error prone and doesn't follow the "best practices" espoused by the redux community and other redux-related packages. Namely:

• redux action names are very general (e.g. 'Reset', 'Init') and likely to collide with other libraries or general redux usage.
• referring to redux action names as strings throughout the codebase instead of setting them as constants (http://redux.js.org/docs/basics/Actions.html#actions)
• manually creating the action object ({ type, payload }) instead of using action creators (http://redux.js.org/docs/basics/Actions.html#action-creators)

This PR addresses all of those issues. This library now exports the action type constants and action creators for each type and uses a 'navigation/' namespace for the types to avoid collisions.

NOTE: I just noticed the "common navigation" spec - cool idea! I still think these actions should be namespaced though and I think the navigation is pretty general.

Test plan (required)

Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes UI.
Make sure you test on both platforms if your change affects both platforms.
The code must pass tests and shouldn't add more Flow errors.

All the existing tests are still passing including new ones for the actions and action creators.

Code formatting

Look around. Match the style of the rest of the codebase.

I followed the coding patterns within the codebase.

[ericvicenti]

In general I am in support of this change, but it is very breaky. This will break for people who use the existing action names, but I do agree that this redux-style action creators are preferable for those who aren't using flow.

So I'm curious if anybody else feels strongly about this.

[jeffcarbs]

Glad you support the change! I'm certainly aware this is breaky. I figure that given this has only been published for ~5 days and it's technically a beta release (1.0.0.beta.1) it'd be more acceptable - better now than when out of beta. That being said, I defer to the contributors here.

One thought: This library only consumes the actions in getStateForAction in StackRouter and TabRouter. We could add a deprecation warning (console.warn) that maps old action types to new ones with a note that says once out of beta the old action types are going to be officially deprecated and to use the action creators instead. This assumes there will be at least one more beta release and I think it's a reasonable compromise.

[jeffcarbs]

@ericvicenti - Just pushed a commit to prevent most of the breakage. So now:

• internally this library will consume/dispatch the new action types.
• when an action with an old type is dispatched from userland we'll map it to the corresponding new type internally (e.g. Init becomes navigation/INIT) along with a warning, e.g.:

The action type 'Init' has been renamed to 'navigation/INIT'. 'Init' will continue to work while in beta but will be removed in the first major release. Moving forward, you should use the action constants and action creators exported by this library in the 'actions' object. See #120 for more details.

The only thing this doesn't handle is a case where a user has a custom router with a custom getStateForAction that is listening for the old actions. This will break since we're no longer dispatching the old actions.

[ericvicenti]

Thanks for adding the informative warning, thats an excellent approach!

Have you run flow on these changes? I know there are a bunch of existing flow issues, but I want to make sure this doesn't introduce more issues.

[jeffcarbs]

There were 110 flow errors on master, 107 on this branch. I also confirmed the playground builds both worked fine but couldn't get the web running locally. The contributing docs refer to a 'prod' npm task that doesn't seem to exist.

[Steviey]

Once React-Navigation is Redux-best practice confirmed, I will start with app coding.
Otherwise there is to much refactoring involved for me later.
Please merge as soon as possible. Thank you for all till now.

[ericvicenti]

Ok, I think this is pretty much ready to land.

@satya164, I agree about the capitalization. Want to clean that up and then ship this thing?

[satya164]

@ericvicenti Sounds good. If @jacrbo doesn't manage to do it sometime, I can take it up :)

[jeffcarbs]

@ericvicenti @satya164 - Currently the constants are all caps (BACK, SET_PARAMS) and the action creators are camelCased (back, setParams). I'm afraid changing the constants to pascal case (Back, SetParams) may make them look too similar to the action creators:

NavigationActions.Back === 'Navigation/Back'
NavigationActions.back() === { type: 'Navigation/Back' }

NavigationActions.SetParams === 'Navigation/SetParams'
NavigationActions.setParams() === { type: 'Navigation/SetParams' }

Two points in favor of ALL_CAPS_SNAKE_CASE:

• that's how action types are typically formatted in the redux docs and almost every redux library I've used.
• it's a very general programming pattern in many different languages to format constant values in this way. PascalCase is typically used for classes or singletons.

I'd be in favor of:

NavigationActions.BACK === 'Navigation/BACK'
NavigationActions.back() === { type: 'Navigation/BACK' }

NavigationActions.SET_PARAMS === 'Navigation/SET_PARAMS'
NavigationActions.setParams() === { type: 'Navigation/SET_PARAMS' }

... but ultimately defer to you guys. Let me know what you think and I'll update the PR real quick.

[satya164]

I'm fine with that too 👍

[jeffcarbs]

I pushed up a commit but missed a few imports. Just fixed and force-pushed so I think what's up there should be good to go 👍

[satya164]

LGTM. @ericvicenti should I merge?

[hilkeheremans]

Great job!

Suggest we close #40, #172 and #153.