Skip to content

bind proper store context in connectAdvanced and the Subscription util #465

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

Merged
merged 2 commits into from
Aug 19, 2016
Merged

bind proper store context in connectAdvanced and the Subscription util #465

merged 2 commits into from
Aug 19, 2016

Conversation

vhmth
Copy link
Contributor

@vhmth vhmth commented Aug 18, 2016

Addresses #464.

The issue explains this fix pretty well, but basically in the previous version of connect the store's context was respected when calling getState or subscribe. I move that we should have that be respected in this version as well.

Also left you a quick video:

https://www.useloom.com/share/2093c4a0652111e6a1ffdd04f7a207b4

@jimbolla
Copy link
Contributor

jimbolla commented Aug 18, 2016

If this is important for some 3rd party libraries, could you also add a test that verifies we're doing the right thing?

@@ -159,7 +159,8 @@ export default function connectAdvanced(
}

initSelector() {
const { dispatch, getState } = this.store
const { dispatch } = this.store
let getState = this.store.getState.bind(this.store)
Copy link
Member

Choose a reason for hiding this comment

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

nit: Please use a const here since this var will not be modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌🏾 will do.

@vhmth
Copy link
Contributor Author

vhmth commented Aug 18, 2016

@jimbolla will do as soon as I get back to my computer.

@vhmth
Copy link
Contributor Author

vhmth commented Aug 18, 2016

Added a test which uses a generic ContextBoundStore class instance. This class can be used in other places as well if need be going forward.

I also cached a bound version of getState within the Connect component in connectAdvanced so that its use is promoted if used elsewhere down the road.

@vhmth
Copy link
Contributor Author

vhmth commented Aug 18, 2016

Out of curiosity, how does the org process of publishing/updating the next package work?

When y'all are good with this, I'd like to update our beta extension right away to solve an issue we have with our users, so just trying to get a time frame there.

@jimbolla
Copy link
Contributor

Can someone assign this to the 5.0 milestone?

@ellbee ellbee added this to the 5.0 milestone Aug 19, 2016
@timdorr
Copy link
Member

timdorr commented Aug 19, 2016

Thanks!

I'll probably publish another release (as beta) once we get the fix for #457 in.

@timdorr timdorr merged commit 77a9aa4 into reduxjs:next Aug 19, 2016
@vhmth
Copy link
Contributor Author

vhmth commented Aug 19, 2016

Sounds good @timdorr. Saw that @jimbolla already has a fix he's playing around with. Again - thanks for the work on this next release guys!

@taion taion mentioned this pull request Nov 1, 2016
albertodev7 pushed a commit to albertodev7/react-redux that referenced this pull request Dec 8, 2022
bind proper store context in connectAdvanced and the Subscription util
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