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

Add tests for the behavior of unsubscribing affecting next dispatch a… #1291

Merged
merged 3 commits into from Jan 28, 2016

Conversation

2 participants
@slorber
Contributor

slorber commented Jan 27, 2016

…nd not current dispatch listeners (decision of gaearon in #1180)

Add tests for the behavior of unsubscribing affecting next dispatch a…
…nd not current dispatch listeners (decision of gaearon in #1180)
store.dispatch(unknownAction())
expect(listener1.calls.length).toBe(1)
expect(listener2.calls.length).toBe(1)
// listener3 is the one removing the others so obviously it got executed

This comment has been minimized.

@gaearon

gaearon Jan 27, 2016

Contributor

I'd still prefer an explicit check to an “obvious” comment ;-)

expect(listener5.calls.length).toBe(1)
})
it('not fire immediately if a listener is added inside another listener', () => {

This comment has been minimized.

@gaearon

gaearon Jan 27, 2016

Contributor

Nit: it('does not

let listener3Added = false
const maybeAddThirdListener = () => {
if ( !listener3Added ) {

This comment has been minimized.

@gaearon

gaearon Jan 27, 2016

Contributor

Nit: we don't put spaces around conditions. (Why doesn't ESLint catch that?)

This comment has been minimized.

@slorber

slorber Jan 27, 2016

Contributor

oooh ok i hate this rule :)

This comment has been minimized.

@gaearon

gaearon Jan 27, 2016

Contributor

FWIW I hate no-semicolons but other folks from rackt use this style. Consistency 💯

// Listener 1 is normal
store.subscribe(() => listener1())
// Listener 2 adds the 3rd listener another listener

This comment has been minimized.

@gaearon

gaearon Jan 27, 2016

Contributor

I think we can remove these comments, the code is pretty descriptive

@gaearon

This comment has been minimized.

Contributor

gaearon commented Jan 27, 2016

Thanks! This is a valuable PR. Always good to ensure we don't break promises.
I'll be happy to merge after you address a few nits.

slorber added some commits Jan 27, 2016

@slorber

This comment has been minimized.

Contributor

slorber commented Jan 27, 2016

@gaearon I've updated the code it should be fine now

np that will be my first merged PR on redux ecosystem :)

gaearon added a commit that referenced this pull request Jan 28, 2016

Merge pull request #1291 from slorber/master
Add tests for the behavior of unsubscribing affecting next dispatch a…

@gaearon gaearon merged commit b4dec4d into reduxjs:master Jan 28, 2016

1 check passed

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

This comment has been minimized.

Contributor

gaearon commented Jan 28, 2016

Appreciated. Congrats!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment