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

Proposal: remove support for emitChange payloads in BaseStore #405

Open
mridgway opened this issue Mar 24, 2016 · 4 comments
Open

Proposal: remove support for emitChange payloads in BaseStore #405

mridgway opened this issue Mar 24, 2016 · 4 comments

Comments

@mridgway
Copy link
Collaborator

Payloads being emitted from emitChange can cause issues where state is actually being emitted rather than letting the store be used as the source of truth. This is problematic because a component may not be set up to listen to a store when it emits the event, then after it has missed the event it renders based on the state of the store.

In short, this prevents imperative view changes based on events and forces developers to use the store state instead.

This would be a major breaking change for people that use these payloads. I'd be curious to hear use cases where they are being used for something that could not be done using store state.

Code that allows this: https://github.com/yahoo/fluxible/blob/master/packages/dispatchr/addons/BaseStore.js#L72

@mridgway mridgway changed the title [dispatchr] Proposal: remove support for emitChange payloads in BaseStore Proposal: remove support for emitChange payloads in BaseStore Mar 24, 2016
@mridgway
Copy link
Collaborator Author

Example:

module.exports = createStore({
    storeName: 'TestStore',
    statics: {
        handlers: {
            'FOO_OPENED': (payload) => {
                this.emitChange({
                    fooHappened: true
                });
            }
        }
    }
});

@armandabric
Copy link
Contributor

I didn't know we can do this. And now I think about it I do not find a case where this could be useful. So I'm in favor you're proposal.

@ali1k
Copy link
Contributor

ali1k commented Mar 25, 2016

I wasn't aware of it either! It sounds like a good decision to disallow this!

@pizzarob
Copy link

I think this is useful for listening for successful requests made from the Fetchr plugin, because there is no really good way to verify a successful response from Fetchr without creating a whole new store for request statuses. For example I want to redirect when I add a new category to my database and then store. Once the category is added to the store I want to listen for success and then redirect based on that. The store is still the source of truth when it comes to the data, but when I get a successful response I know to redirect and then forget about it.

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

No branches or pull requests

4 participants