Skip to content

Ensure context of dispatch #533

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

Closed
wants to merge 1 commit into from
Closed

Conversation

kevinresol
Copy link

I am trying to port Redux into the Haxe language with the same compatible API. But due to the difference in the internal structure there are some context error when the port is used with react-redux. And this patch fixes that.

This patch shouldn't affect existing react-redux user, because the original Redux implemented dispatch as a closure over the internal state object and doesn't reference this at all. But please feel free to close if it is deemed not necessary. Thanks.

@markerikson
Copy link
Contributor

Can you clarify what the actual problem is that this tries to solve?

My gut says that there's been a couple past PRs to either Redux or React-Redux that tried to fiddle with binding for either dispatch or subscribe, but a quick search isn't turning them up atm. Pretty sure this has been discussed in the past and been turned down, although I could be wrong.

@kevinresol
Copy link
Author

As I said, I am try to re-implement Redux and another programming language/transpiler. I implemented the store as a class and put the state as a member variable. As a result, the state object is referenced as this.state inside the dispatch function. React-redux passes the dispatch function around without caring about the context (and it doesn't need to, as mentioned in my original post) and that causes problem in my class-based implementation. I see that I can implement it the same way redux does, which is not really big deal. But I also think that patching here shouldn't be a big deal either (well, except breaking a couple equality tests because the bound dispatch is not the original dispatch any more). So... well, yeah, that's it.

@taion
Copy link

taion commented Nov 1, 2016

Speaking from a position of zero knowledge (THAT MEANS IGNORE THE ORG MEMBERSHIP BADGE), it seems inconsistent to me that getState is bound, while dispatch isn't.

@markerikson
Copy link
Contributor

Hmm. Are you sure? It doesn't look like it is. It's defined free-floating at createStore#L74, and returned at line 250. No binding of any kind going on there.

@taion
Copy link

taion commented Nov 1, 2016

Sorry – I'm referring specifically to https://github.com/reactjs/react-redux/blob/v5.0.0-beta.3/src/components/connectAdvanced.js#L109-L111, as well as the test cases added in #465.

@timdorr
Copy link
Member

timdorr commented Nov 1, 2016

For consistency, this should be done against the next branch instead, where we're binding getState, as @taion pointed out. Plus, it would be a good stress test to try and use that new branch with a reimplementation of Redux 😄

@timdorr timdorr closed this Nov 1, 2016
@taion
Copy link

taion commented Nov 1, 2016

That's actually just matching the behavior in release, though – it's just that on the released branch, it happens implicitly because the only access to store state is by calling store.getState().

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.

4 participants