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

Provide action to subscribed functions. #120

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AndrewSouthpaw
Copy link

It's useful among subscribers to know which action is being dispatched. This PR provides the action object to subscribed functions.

Copy link
Contributor

@ohtangza ohtangza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code itself should work well with this test.

One thing I worried is that this library does not seem to guarantee the full functionality of redux's store.

@AndrewSouthpaw
Copy link
Author

Thanks for the review, @ohtangza.

I would love to get this merged so I can stop using my forked version of this repo. 😆

@ohtangza
Copy link
Contributor

@AndrewSouthpaw My pleasure, let's wait for @arnaudbenard response then.

@crazy4groovy
Copy link

I'd love to see this happen.
Actually I was hoping it would follow the reducer API:
store.subscribe((state, action) => console.log('!!DISPATCH!!!', state, action));
But I guess the mock state never changes, so just having action is fine, too..

@arnaudbenard
Copy link
Contributor

I don't think it follows the redux design, check this https://github.com/reactjs/redux/blob/master/docs/faq/DesignDecisions.md#does-not-pass-state-action-to-subscribers

@dmitry-zaets what do you think?

@AndrewSouthpaw
Copy link
Author

That's true... it does break from the design.

I thought it wouldn't be critical to follow that same design choice, since this is a mock store intended for testing purposes. The state never changes anyway, so the concept of batching isn't particularly relevant, while it can be useful in tests to subscribe to particular actions firing in the store.

@AndrewSouthpaw
Copy link
Author

Ping. :)

@@ -38,7 +38,7 @@ export default function configureStore (middlewares = []) {
actions.push(action)

for (let i = 0; i < listeners.length; i++) {
listeners[i]()
listeners[i](action)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this action be passed in here after removing the type property?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there's a huge demand for it, I'd leave the type property, it gives more rich info to any listeners in test.

@abe4mvp
Copy link

abe4mvp commented May 22, 2018

If not this solution, is there an alternative way that you could still get the state passed in. that would be helpful in testing components that are wrapped in connect() call because it would allow you to test your mapStateToProps function without having to export it publicly. instead you simply dispatch an action and assert on the props of the connected component

@AndrewSouthpaw
Copy link
Author

@abe4mvp If I understand you correctly, that would require a working reducer to be hooked into the store, which is currently not possible. I've actually extended this repo to allow that, so I find it helpful, but... yeah. Not sure if that's beyond what would be considered "proper" for this repo.

@AndrewSouthpaw
Copy link
Author

Ping?

@jednano
Copy link

jednano commented Jul 12, 2019

I implemented this in my TypeScript fork @jedmao/redux-mock-store if you want to take a look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants