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

feat(EpicMiddleware): warn about improper use of dispatch function #336

Merged
merged 5 commits into from Oct 11, 2017

Conversation

@evertbouw
Copy link
Member

commented Oct 10, 2017

closes #314

@jayphelps
Copy link
Member

left a comment

Yay! This is awesome. Thank you for spearheading this.

const vault = (process.env.NODE_ENV === 'production') ? store : {
getState: store.getState,
dispatch: (action) => {
store.dispatch(action);

This comment has been minimized.

Copy link
@jayphelps

jayphelps Oct 10, 2017

Member

We need to capture the return value of store.dispatch() and return it from our wrapper, in case they have another middleware that returns something meaningful like redux-thunk, etc.

Something like

const result = store.dispatch(action);
console.warn(...);
return result;
getState: store.getState,
dispatch: (action) => {
store.dispatch(action);
console.warn(`Your Epic "${epic.name || '<anonymous>'}" called store.dispatch directly. This is an anti-pattern.`);

This comment has been minimized.

Copy link
@jayphelps

jayphelps Oct 10, 2017

Member

epic.name refers to the root epic in this scope I think? export function createEpicMiddleware(epic, ... If so, it'll probably be confusing. You probably had this code originally inside the ::map(epic => { scope, in which case epic refers to the actual epic that did use store.dispatch, but you'd have to create a "vault" for each epic, which I think is an ok tradeoff. 😄

While you're in here, you might change the root epic variable from epic to rootEpic to make it less confusing and prevent our ::map(epic => { from shadowing it.

This comment has been minimized.

Copy link
@evertbouw

evertbouw Oct 11, 2017

Author Member

Hmm I moved the vault into the ::map but it seems the epic there is the root epic, and the name undefined. Maybe I can use the action.type in the warning message instead?

(I chose "vault" as a name for a store with more restrictions)

This comment has been minimized.

Copy link
@jayphelps

jayphelps Oct 11, 2017

Member

Ohhh right, I forgot that inside the middleware we don't have access to the individual epics because they were composed with combineEpics. I think I'll adjust it to just be generic and not mention which specific epic is to blame. I'm also going to change the message a bit to clarify that we're actually deprecating this (store.dispatch inside epics) and it will be removed in v1.0.0.

done();
};
const store = createStore(reducer, applyMiddleware(epicMiddleware, mockMiddleware));
store.dispatch({ type: 'FIRST_ACTION_TO_TRIGGER_MIDDLEWARE' });
});

it('should warn about improper use of dispatch function', () => {
sinon.spy(console, 'warn');

This comment has been minimized.

Copy link
@jayphelps

jayphelps Oct 10, 2017

Member

Missing a matching restore of the original at the end of the test, console.warn.restore()


store.dispatch({ type: 'PING' });

expect(console.warn.called).to.equal(true);

This comment has been minimized.

Copy link
@jayphelps

jayphelps Oct 10, 2017

Member

Can you change to expect(console.warn.callCount).to.equal(1); or something similar--just want to make sure console.warn is only called once.

@jayphelps jayphelps merged commit 76ecd33 into redux-observable:master Oct 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jayphelps

This comment has been minimized.

Copy link
Member

commented Oct 11, 2017

🎉 woot! thank you @evertbouw!! Very much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.