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

HMR: connected component gets disconnected from store #670

Closed
hoschi opened this issue Apr 12, 2017 · 16 comments · Fixed by #715
Closed

HMR: connected component gets disconnected from store #670

hoschi opened this issue Apr 12, 2017 · 16 comments · Fixed by #715

Comments

@hoschi
Copy link

hoschi commented Apr 12, 2017

Description

Connected component gets disconnected from store when code changed and applied by HMR.

Steps:

Expected behavior

Remove <p>test</p> and 'Increment' component should receive changed state when clicked on one of the buttons.

Actual behavior

When I remove <p>test</p> HMR does its code updating which leads to the problem that 'Increment' component doesn't get new store state. You can click on the buttons and the counter stays the same. I can see the actions dispatched in Redux devtools.

I found out that this is the case when the component above in the hierarchy is a container component which subscribed to state which is not subscribed in other components in the tree. You don't get the problem when you change
https://github.com/hoschi/bare-minimum-react-hot-rr4-redux/blob/08740ca8bac9a2d2d7e7527346f0ae36dde3234e/src/components/AppBarDesktopContainer.jsx
to:

import { connect } from 'react-redux';
import AppBarDesktop from './AppBarDesktop';

export default connect((state) => ({
    newsData:state.counter
}))(AppBarDesktop);

and do the same steps described in the Description part.

Environment

react-redux 5.0.4, I tried also 5.0.2 and 5.0.3 without effect
https://github.com/hoschi/bare-minimum-react-hot-rr4-redux/blob/08740ca8bac9a2d2d7e7527346f0ae36dde3234e/package.json

Run these commands in the project folder and fill in their results:

  1. node -v: 6.3.1
  2. npm -v: 3.10.3

Then, specify:

  1. Operating system: Linux (Arch)
  2. Browser and version: Chrome, 56.0.2924.87 (64-bit)

Reproducible Demo

https://github.com/hoschi/bare-minimum-react-hot-rr4-redux/tree/disconnected-component

Probably related issues

fun fact: You have to delete the tag below, deleting one above will trigger this bug and 'fixes' the one I'm reporting here.

@ebradshaw
Copy link

I can confirm this with react-redux 5.04, 5.03 and 5.02, although 5.01 seems to be working for me.

@gaearon
Copy link
Contributor

gaearon commented Apr 13, 2017

Does this happen with 4.x? I see quite a few issues with HMR reported after 5.x was released. If you don't rely on any 5.x specific features maybe it would be better to wait out and use 4.x until these are fixed.

@lukescott
Copy link

lukescott commented Apr 14, 2017

Perhaps I'm missing something, but your store doesn't have module.hot.accept or store.replaceReducer:

import { createStore } from 'redux';

const reducer = (state = {
	counter:0,
	newsData:'my data'
}, action) => {
    switch (action.type) {
        case 'INCREMENT':
            return Object.assign({}, state, {
				counter:state.counter +1
			})
        case 'DECREMENT':
            return Object.assign({}, state, {
				counter:state.counter -1
			})
    }

    return state;
}

export default createStore(reducer, window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__());

https://github.com/hoschi/bare-minimum-react-hot-rr4-redux/blob/08740ca8bac9a2d2d7e7527346f0ae36dde3234e/src/store.js

My store creator looks like this:

import {createStore} from "redux";
import rootReducer from "../reducers";
import {devToolsEnhancer} from "redux-devtools-extension/developmentOnly";

export default function configureStore(state) {
	const store = createStore(rootReducer, state, devToolsEnhancer());
	if (module.hot) {
		module.hot.accept("../reducers", () => {
			// I have "es2015": {"modules": false} on my .babelrc -
			// if you don't have this you need:
			// store.replaceReducer(require("../reducers").default);
			store.replaceReducer(rootReducer);
		});
	}
	return store;
}

That worked for me. As far as I know you need module.hot.accept for both React and Redux.

@markerikson
Copy link
Contributor

The project listed above, and the (presumably same) issue listed in #636, are using React-Hot-Loader. RHL works by transforming your code and inserting module.hot.accept calls around each component class, so that you can swap out individual component classes rather than the entire component tree.

There's enough "me too!"s on these two threads to make it evident that this is a real bug, and presumably introduced in one of the 5.0.x patches. I'm returning from travel this weekend. I will try to make it a point to look into this within the next week. (If @jimbolla wants to look into it first, he knows this code much better than I do, but I'm willing to take a shot at debugging it.)

@markerikson
Copy link
Contributor

Hmm. Been playing around with this a bit. @hoschi , definitely appreciate the minimal repro example - that helps a lot.

What I'm seeing at the moment is that if I modify the contents of AppBarDesktop per the repro instructions, AppBarDesktop gets hot-reloaded and has its version value incremented. However, Increment does not have its hot reloading version counter incremented, so it doesn't try to re-initialize its subscription, and presumably is still subscribed to something that isn't firing updates any more.

Still trying to familiarize myself with the code and what's actually going on here, but that's at least a bit of useful info. Also not sure how much this relates to #636 , particularly given that I tried this with 5.0.0 and saw the same behavior.

@hoschi
Copy link
Author

hoschi commented Apr 23, 2017 via email

@markerikson
Copy link
Contributor

Huh. So I'm playing with npm link for the first time (or technically, yarn link). I cloned react-redux and linked it into the sample project... and now, even though the Increment version still isn't bumping, it seems to be maintaining its subscription and the counter increments correctly after a hot reload of AppBarDesktop. Go figure.

@timdorr
Copy link
Member

timdorr commented Apr 24, 2017

Does yarn link even work yet? I thought they were still implementing that one. I'd recommend using npm link for now to avoid any distraction.

@markerikson
Copy link
Contributor

It certainly appears to work. It created a junction/hardlink on my Win7 machine, and the repro project showed the "test comment" I left in connectAdvanced.js as a confirmation.

@markerikson
Copy link
Contributor

markerikson commented Apr 24, 2017

And even more weirdly, if I unlink with yarn and link with NPM, I see the bug again.

¯\_(ツ)_/¯

Back to poking around at this...

@markerikson
Copy link
Contributor

What I think is going on, and this is still a hazy half-informed guess, is that the AppBarDesktop component is hot-reloaded, but the Increment component isn't. (Which makes sense - we're editing AppBarDesktop.) This means that Increment is still subscribed to the AppBarDesktop.1's Subscription instance, but that's been thrown away as AppBarDesktop.2 is created. As a result, Connect(Increment)'s dev-mode componentWillUpdate never gets executed, and it doesn't even know to try to re-create its subscription.

It seems like we might need some way for a child to detect that the parent subscription is no longer valid,, but I'm not immediately sure how to accomplish that or where to do the check.

@markerikson
Copy link
Contributor

(Also, half the time the code runs fine if I've got react-redux linked, either via yarn or via NPM. ?!?!? )

@lukescott
Copy link

What happens if you remove IncrementContainer and DecrementContainer and just had the top-level connect?

@markerikson
Copy link
Contributor

Haven't tried it yet, but I would expect the counter to update properly (assuming that top container mapped the value and passed it down). The issue appears to be the nested connected component not getting hot-reloaded, and thus losing its subscription.

@lukescott
Copy link

lukescott commented Apr 24, 2017

Not sure if this is relevant, but I noticed mapStateToPropsis undefined on DecrementContainer, but provided on IncrementContainer. Which could mean DecrementContainer would not get updated when reloaded because it isn't subscribed:

// if mapStateToProps is falsy, the Connect component doesn't subscribe to store state changes
shouldHandleStateChanges: Boolean(mapStateToProps),

https://github.com/reactjs/react-redux/blob/2000ff54aef486d955a7eb4fe9594e0b5e273458/src/connect/connect.js#L71-L72

export default connect(undefined, mapDispatchToProps)(Decrement)
export default connect(mapStateToProps, mapDispatchToProps)(Increment)

@nihiluis
Copy link

nihiluis commented Jun 4, 2017

i think in this repo it's working fine with 5.0.4 and react hot loader 1.3.1
https://github.com/rokoroku/react-redux-typescript-boilerplate/blob/master/package.json

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 a pull request may close this issue.

7 participants