-
Notifications
You must be signed in to change notification settings - Fork 149
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
base: master
Are you sure you want to change the base?
Provide action to subscribed functions. #120
Conversation
There was a problem hiding this 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.
Thanks for the review, @ohtangza. I would love to get this merged so I can stop using my forked version of this repo. 😆 |
@AndrewSouthpaw My pleasure, let's wait for @arnaudbenard response then. |
I'd love to see this happen. |
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? |
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. |
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 |
@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. |
Ping? |
I implemented this in my TypeScript fork |
It's useful among subscribers to know which action is being dispatched. This PR provides the action object to subscribed functions.