Skip to content

Commit

Permalink
normalize action name with prefix and expose it from module (#900)
Browse files Browse the repository at this point in the history
* Update Actions.js

prefixed action strings with 'router_' to namespace them.

I noticed the action constants are very generic strings (not namespaced like 'router_jump' etc.):
```
export const JUMP_ACTION = 'jump';
export const PUSH_ACTION = 'push';
export const REPLACE_ACTION = 'replace';
export const POP_ACTION2 = 'back';
export const POP_ACTION = 'BackAction';
export const REFRESH_ACTION = 'refresh';
export const RESET_ACTION = 'reset';
export const FOCUS_ACTION = 'focus';
```
As I understand Redux, all actions get passed to all reducers when you use `combineReducers`. We see these strings as being so generic that it opens a very real risk of me (or another module I did not read) dispatching an action that evaluates to the same string, triggering an action that was not supposed to be triggered. To prevent this I propose the following change, or any other that diminishes this risk.

* Update Reducer.test.js to reflect new action strings

* action strings in Reducer.test.js

* Update action strings as per discussion

*  Update action strings as per discussion

* normalize action name with prefix and expose it from module

This is a follow up of #843 which has been reverted (#899) since it breaks
existig apps as #894 described.

change description:

  * all actions now defined in a separate file: `ActionConst.js`.
  * create an mapping object called: `ActionMap` in `Actions.js`,
    it maps deprecated string literal actions to constant one.

router:

  * will dispatch constant actions if we have dispatch method in props
  * will pass unmodified type into onNavigate.

reducer:

  * always use ActionConst and ActionMap to determine whether an action type matched or not

know issue:

  it's really hard to not break ANY existing app.
  especially for those who:
    1. stores routing status outside from RNRF
    2. use custom reducer instead of RNRF default reducer
  should be always use action constant from RNRF.

it will NOT break if user uses RNRF default reducer.

* update Example to use ActionConst

* change documents related to action type name
  • Loading branch information
zxcpoiu authored and aksonov committed Jul 8, 2016
1 parent b7ee480 commit 1ed01fc
Show file tree
Hide file tree
Showing 10 changed files with 138 additions and 82 deletions.
5 changes: 3 additions & 2 deletions Example/Example.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
Switch,
Modal,
Actions,
ActionConst,
} from 'react-native-router-flux';
import Error from './components/Error';
import Home from './components/Home';
Expand Down Expand Up @@ -92,7 +93,7 @@ const SwitcherPage = (props) => (
</Button>
<Button
onPress={() => {
Actions.launch({ type: 'reset' });
Actions.launch({ type: ActionConst.RESET });
}}
>
Exit
Expand Down Expand Up @@ -131,7 +132,7 @@ class Example extends Component {
</Scene>
<Scene key="register" component={Register} title="Register" />
<Scene key="register2" component={Register} title="Register2" duration={1} />
<Scene key="home" component={Home} title="Replace" type="replace" />
<Scene key="home" component={Home} title="Replace" type={ActionConst.REPLACE} />
<Scene key="launch" component={Launch} title="Launch" initial />
<Scene key="login" direction="vertical" >
<Scene key="loginModal" direction="vertical" component={Login} title="Login" />
Expand Down
33 changes: 32 additions & 1 deletion docs/API_CONFIGURATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- `DefaultRenderer`
- `Switch`
- `Actions`
- `ActionConst`
- `NavBar`

## Router:
Expand All @@ -29,9 +30,39 @@
| key | `string` | required | Will be used to call screen transition, for example, `Actions.name(params)`. Must be unique. |
| component | `React.Component` | semi-required | The `Component` to be displayed. Not required when defining a nested `Scene`, see example. If it is defined for 'container' scene, it will be used as custom container `renderer` |
| initial | `bool` | false | Set to `true` if this is the initial scene |
| type | `string` | 'push' or 'jump' | Defines how the new screen is added to the navigator stack. One of `push`, `jump`, `replace`, `reset`. If parent container is tabbar (tabs=true), jump will be automatically set.
| type | `string` | `ActionConst.PUSH` or `ActionConst.JUMP` | Defines how the new screen is added to the navigator stack. One of `ActionConst.PUSH`, `ActionConst.JUMP`, `ActionConst.REPLACE`, `ActionConst.RESET`. If parent container is tabbar (tabs=true), `ActionConst.JUMP` will be automatically set.
| clone | `bool` | | Scenes marked with `clone` will be treated as templates and cloned into the current scene's parent when pushed. See example. |
| passProps | `bool` | false | Pass all own props (except style, key, name, component, tabs) to children. Note that passProps is also passed to children. |

## ActionConst:

We accept shorthand string literal when defining scene tpye or action params, like:

```javascript
Actions.ROUTE_NAME({type: 'reset'});
<Scene key="myscene" type="replace" >
```

but it will be converted to const value when pass to reducer.
RECOMMENDATION is to always use const instead of string literal for consistency:

```javascript
Actions.ROUTE_NAME({type: ActionConst.RESET});
<Scene key="myscene" type={ActionConst.REPLACE} >
```

| Property | Type | Value | Shorthand |
|-----------|--------|---------|-----------------------------------------|
| ActionConst.JUMP | `string` | 'REACT_NATIVE_ROUTER_FLUX_JUMP' | 'jump' |
| ActionConst.PUSH | `string` | 'REACT_NATIVE_ROUTER_FLUX_PUSH' | 'push' |
| ActionConst.REPLACE | `string` | 'REACT_NATIVE_ROUTER_FLUX_REPLACE' | 'replace' |
| ActionConst.BACK | `string` | 'REACT_NATIVE_ROUTER_FLUX_BACK' | 'back' |
| ActionConst.BACK_ACTION | `string` | 'REACT_NATIVE_ROUTER_FLUX_BACK_ACTION' | 'BackAction' |
| ActionConst.POP_TO | `string` | 'REACT_NATIVE_ROUTER_FLUX_POP_TO' | 'popTo' |
| ActionConst.REFRESH | `string` | 'REACT_NATIVE_ROUTER_FLUX_REFRESH' | 'refresh' |
| ActionConst.RESET | `string` | 'REACT_NATIVE_ROUTER_FLUX_RESET' | 'reset' |
| ActionConst.FOCUS | `string` | 'REACT_NATIVE_ROUTER_FLUX_FOCUS' | 'focus' |

### Animation
| Property | Type | Default | Description |
|-----------|--------|---------|--------------------------------------------|
Expand Down
7 changes: 5 additions & 2 deletions docs/DETAILED_EXAMPLE.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
## Detailed Example

for latest example code, please see [Example](https://github.com/aksonov/react-native-router-flux/blob/master/Example/Example.js)

![launch](https://cloud.githubusercontent.com/assets/1321329/11692367/7337cfe2-9e9f-11e5-8515-e8b7a9f230ec.gif)

```jsx
Expand All @@ -7,7 +10,7 @@ import Launch from './components/Launch'
import Register from './components/Register'
import Login from './components/Login'
import Login2 from './components/Login2'
import {Scene, Router, TabBar, Modal, Schema, Actions, Reducer} from 'react-native-router-flux'
import { Scene, Router, TabBar, Modal, Schema, Actions, Reducer, ActionConst } from 'react-native-router-flux'
import Error from './components/Error'
import Home from './components/Home'
import TabView from './components/TabView'
Expand Down Expand Up @@ -35,7 +38,7 @@ export default class Example extends React.Component {
<Scene key="root" hideNavBar={true}>
<Scene key="register" component={Register} title="Register"/>
<Scene key="register2" component={Register} title="Register2" duration={1}/>
<Scene key="home" component={Home} title="Replace" type="replace"/>
<Scene key="home" component={Home} title="Replace" type={ActionConst.REPLACE}/>
<Scene key="launch" component={Launch} title="Launch" initial={true} style={{flex:1, backgroundColor:'transparent'}}/>
<Scene key="login" direction="vertical">
<Scene key="loginModal" component={Login} schema="modal" title="Login"/>
Expand Down
4 changes: 3 additions & 1 deletion docs/REDUX_FLUX.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,16 @@ First create a reducer for the routing actions that will be dispatched by RNRF.
```javascript
// reducers/routes.js

import { ActionConst } from 'react-native-router-flux';

const initialState = {
scene: {},
};

export default function reducer(state = initialState, action = {}) {
switch (action.type) {
// focus action is dispatched when a new screen comes into focus
case "focus":
case ActionConst.FOCUS:
return {
...state,
scene: action.scene,
Expand Down
2 changes: 2 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Actions from './src/Actions';
import * as ActionConst from './src/ActionConst';
import DefaultRenderer from './src/DefaultRenderer';
import Modal from './src/Modal';
import NavBar from './src/NavBar';
Expand All @@ -12,6 +13,7 @@ import Util from './src/Util';

export {
Actions,
ActionConst,
DefaultRenderer,
Modal,
NavBar,
Expand Down
9 changes: 9 additions & 0 deletions src/ActionConst.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export const JUMP = 'REACT_NATIVE_ROUTER_FLUX_JUMP';
export const PUSH = 'REACT_NATIVE_ROUTER_FLUX_PUSH';
export const REPLACE = 'REACT_NATIVE_ROUTER_FLUX_REPLACE';
export const BACK = 'REACT_NATIVE_ROUTER_FLUX_BACK';
export const BACK_ACTION = 'REACT_NATIVE_ROUTER_FLUX_BACK_ACTION';
export const POP_TO = 'REACT_NATIVE_ROUTER_FLUX_POP_TO';
export const REFRESH = 'REACT_NATIVE_ROUTER_FLUX_REFRESH';
export const RESET = 'REACT_NATIVE_ROUTER_FLUX_RESET';
export const FOCUS = 'REACT_NATIVE_ROUTER_FLUX_FOCUS';
63 changes: 34 additions & 29 deletions src/Actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,28 @@
*/
import { assert } from './Util';
import Scene from './Scene';
export const JUMP_ACTION = 'jump';
export const PUSH_ACTION = 'push';
export const REPLACE_ACTION = 'replace';
export const POP_ACTION2 = 'back';
export const POP_ACTION = 'BackAction';
export const POP_TO = 'popTo';
export const REFRESH_ACTION = 'refresh';
export const RESET_ACTION = 'reset';
export const FOCUS_ACTION = 'focus';
import * as ActionConst from './ActionConst';

export const ActionMap = {
jump: ActionConst.JUMP,
push: ActionConst.PUSH,
replace: ActionConst.REPLACE,
back: ActionConst.BACK,
BackAction: ActionConst.BACK_ACTION,
popTo: ActionConst.POP_TO,
refresh: ActionConst.REFRESH,
reset: ActionConst.RESET,
focus: ActionConst.FOCUS,
[ActionConst.JUMP]: ActionConst.JUMP,
[ActionConst.PUSH]: ActionConst.PUSH,
[ActionConst.REPLACE]: ActionConst.REPLACE,
[ActionConst.BACK]: ActionConst.BACK,
[ActionConst.BACK_ACTION]: ActionConst.BACK_ACTION,
[ActionConst.POP_TO]: ActionConst.POP_TO,
[ActionConst.REFRESH]: ActionConst.REFRESH,
[ActionConst.RESET]: ActionConst.RESET,
[ActionConst.FOCUS]: ActionConst.FOCUS,
};

function filterParam(data) {
if (data.toString() !== '[object Object]') {
Expand All @@ -31,19 +44,11 @@ function filterParam(data) {
}

const reservedKeys = [
POP_ACTION,
POP_ACTION2,
POP_TO,
REFRESH_ACTION,
REPLACE_ACTION,
JUMP_ACTION,
PUSH_ACTION,
FOCUS_ACTION,
RESET_ACTION,
'create',
'callback',
'iterate',
'current',
...Object.keys(ActionMap),
];

function getInheritProps(props) {
Expand Down Expand Up @@ -72,9 +77,9 @@ class Actions {
`'${key}' is not allowed as key name. Reserved keys: [${reservedKeys.join(', ')}]`
);
const { children, component, ...staticProps } = root.props;
let type = root.props.type || (parentProps.tabs ? JUMP_ACTION : PUSH_ACTION);
let type = root.props.type || (parentProps.tabs ? ActionConst.JUMP : ActionConst.PUSH);
if (type === 'switch') {
type = JUMP_ACTION;
type = ActionConst.JUMP;
}
const inheritProps = getInheritProps(parentProps);
const componentProps = component ? { component: wrapBy(component) } : {};
Expand Down Expand Up @@ -117,7 +122,7 @@ class Actions {
list = normalized; // normalize the list of scenes

const condition = el => (!el.props.component && !el.props.children && !el.props.onPress &&
(!el.props.type || el.props.type === REFRESH_ACTION));
(!el.props.type || ActionMap[el.props.type] === ActionConst.REFRESH));
// determine sub-states
let baseKey = root.key;
let subStateParent = parentProps.key;
Expand All @@ -135,7 +140,7 @@ class Actions {
baseKey = innerKey;
subStateParent = res.key;
const inner = { ...res, name: key, key: innerKey,
sceneKey: innerKey, type: PUSH_ACTION, parent: res.key };
sceneKey: innerKey, type: ActionConst.PUSH, parent: res.key };
refs[innerKey] = inner;
res.children = [innerKey];
delete res.component;
Expand All @@ -144,15 +149,15 @@ class Actions {
}
// process substates
for (const el of subStates) {
refs[el.key] = { key: el.key, name: el.key, ...el.props, type: REFRESH_ACTION,
refs[el.key] = { key: el.key, name: el.key, ...el.props, type: ActionConst.REFRESH,
base: baseKey, parent: subStateParent };
if (this[el.key]) {
console.log(`Key ${el.key} is already defined!`);
}
this[el.key] =
(props = {}) => {
assert(this.callback, 'Actions.callback is not defined!');
this.callback({ key: el.key, type: REFRESH_ACTION, ...filterParam(props) });
this.callback({ key: el.key, type: ActionConst.REFRESH, ...filterParam(props) });
};
}
if (this[key]) {
Expand All @@ -169,23 +174,23 @@ class Actions {
}

popTo(props = {}) {
return this.callback({ ...filterParam(props), type: POP_TO });
return this.callback({ ...filterParam(props), type: ActionConst.POP_TO });
}

pop(props = {}) {
return this.callback({ ...filterParam(props), type: POP_ACTION });
return this.callback({ ...filterParam(props), type: ActionConst.BACK_ACTION });
}

jump(props = {}) {
return this.callback({ ...filterParam(props), type: JUMP_ACTION });
return this.callback({ ...filterParam(props), type: ActionConst.JUMP });
}

refresh(props = {}) {
return this.callback({ ...filterParam(props), type: REFRESH_ACTION });
return this.callback({ ...filterParam(props), type: ActionConst.REFRESH });
}

focus(props = {}) {
return this.callback({ ...filterParam(props), type: FOCUS_ACTION });
return this.callback({ ...filterParam(props), type: ActionConst.FOCUS });
}

create(scene:Scene, wrapBy = x => x) {
Expand Down
Loading

0 comments on commit 1ed01fc

Please sign in to comment.