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

Get this working properly with React Router #249

Closed
gaearon opened this Issue Apr 18, 2016 · 46 comments

Comments

Projects
None yet
@gaearon
Copy link
Owner

gaearon commented Apr 18, 2016

There is a hacky workaround in gaearon/react-hot-boilerplate#61 (comment) but it’s way too hacky. We should either implement this in ReactTraining/react-router#2182, or include some sort of hacky workaround right in this project so at least it’s on us. Special casing React Router doesn’t sound very great but it’s hugely popular and we can’t just not support it.

@gaearon gaearon added the enhancement label Apr 18, 2016

@gaearon gaearon added this to the v3.0 milestone Apr 18, 2016

@glenjamin

This comment has been minimized.

Copy link

glenjamin commented Apr 18, 2016

I ran into this today when using react router properly for the first time, and was surprised that no-one had tackled this before.

How do all those people work with a Router that won't hot reload without getting really frustrated?

Ideally, the proper fix for this should be in resolving ReactTraining/react-router#2182 - bcause the warning here is just cheeky: https://github.com/reactjs/react-router/blob/master/modules/Router.js#L131 "Sorry, this looks like a declarative component but it's actually an imperative API call and we're doing to ignore your new routes!"

I'll take my further comments / help over to that thread now 😄

@gaearon

This comment has been minimized.

Copy link
Owner Author

gaearon commented Apr 18, 2016

Previous versions of React Hot Loader or React Transform used to wrap class in the proxy during its definition time, so this wasn’t much of an issue—the update would just never propagate up to the router. The current approach has many more upsides (e.g. works with functional components) but it now wraps components in createElement() so now it needs all components in chain to update the React way, so the Router behavior becomes problematic.

@gaearon

This comment has been minimized.

Copy link
Owner Author

gaearon commented Apr 18, 2016

Thanks for looking at it!

@glenjamin

This comment has been minimized.

Copy link

glenjamin commented Apr 18, 2016

I just had an idea I wanted to write down, not thought through heavily yet.

What if instead of trying to patch <Router>, you patch <Route> to apply the proxy code.

Something along these lines:

// Make a note of the Route component so we can detect it later
require('react-hot-loader/patch-router')(Route);

// ...
React.createElement = function(type, props, ..children) { 
  // as well as all the other stuff
 // When we see a Route, proxy its component
  if (type == Route && props.component) props.component = proxy(props.component)
  // ...
}

This sort of approach would ensure that the component is proxied whenever the routes list is "rendered" even though those <Route> children are never actually applied to <Router>

Exactly how and when to do this is up for debate, but I think the general idea of opt-in to detecting Route and converting those components to proxies should sort things out.

@gaearon

This comment has been minimized.

Copy link
Owner Author

gaearon commented Apr 18, 2016

@glenjamin

Thanks for the idea! I added this to 3.0.0-alpha.12 so people can get started more quickly. This is still ugly though so we’d better figure out a proper way to do this before the release. The router console error stays so hopefully this will bring more light to ReactTraining/react-router#2182 😄 .

@alexanderchr

This comment has been minimized.

Copy link
Contributor

alexanderchr commented Apr 19, 2016

Will have to figure out some way to get this working with getComponent as well

@gaearon

This comment has been minimized.

Copy link
Owner Author

gaearon commented Apr 19, 2016

I feel this is out of scope of this hack and should be fixed in ReactTraining/react-router#2182.

@jaredpalmer

This comment has been minimized.

Copy link

jaredpalmer commented Apr 26, 2016

@gaearon This may be a hack, but passing Router a random key seems to work fine.

// Root.js
import React, { Component } from 'react';
import { Provider } from 'react-redux'
import { browserHistory, Router} from 'react-router'
import configureStore from './store/configureStore'
import routes from './routes'

export default class Root extends Component {
  render() {
    const store = configureStore()
    return (
      <Provider store={store}>
        <Router history={browserHistory} routes={routes} key={Math.random()}/>
      </Provider>
    );
  }
}
@gaearon

This comment has been minimized.

Copy link
Owner Author

gaearon commented Apr 26, 2016

@jaredpalmer As far as I can see, this means a different component is mounted every time, so the state of any children components is destroyed. If this works well for you (e.g. you don’t use local state a lot), you can remove React Hot Loader and just use Webpack HMR API without it.

This was referenced May 1, 2016

@gaearon

This comment has been minimized.

Copy link
Owner Author

gaearon commented May 2, 2016

Aside from the annoying warning, should be fixed in v3.0.0-beta.1.

@burkhardr

This comment has been minimized.

Copy link

burkhardr commented May 3, 2016

Is there a working example for react-router somewhere?

@gaearon

This comment has been minimized.

Copy link
Owner Author

gaearon commented May 3, 2016

@burkhardr

Root.js

export default function Root() {
  return (
    <Router component={App}>
       ...
    </Router>
  )
}

index.js

Take this file and replace App with Root.

@phyllisstein

This comment has been minimized.

Copy link
Contributor

phyllisstein commented May 4, 2016

Sorry if this is a dense question, but is beta.1 expected to still be broken for async routes? I've been trying to make the switch from react-transform-hmr in a project of mine, and I'm currently in a state where hot reloading works for individual components but not the asynchronously-loaded views (even with Webpack code splitting disabled).

@gaearon

This comment has been minimized.

Copy link
Owner Author

gaearon commented May 4, 2016

It’s expected to work (as in, I don’t see why it wouldn’t), but I have not tested it, so issues might still exist.
I would appreciate if you filed an issue with a link to the branch I can test and instructions to reproduce.

@tomitrescak

This comment has been minimized.

Copy link

tomitrescak commented May 19, 2016

Is React Router supposed to work with beta.2 with no errors? If so, what is the way of having it work? The hot reload works for me, but I keep receiving the error in console. The error is:

Warning: [react-router] You cannot change <Router routes>; it will be ignored

[EDIT] The only way to make it work is via setting <Router key={Math.random()} />

@gaearon

This comment has been minimized.

Copy link
Owner Author

gaearon commented May 19, 2016

The hot reload works for me, but I keep receiving the error in console. The error is:

It is not an error, it is a warning. There is no harm in it. Yes, it’s expected to appear until React Router fixes ReactTraining/react-router#2182, but you don’t need to worry about it.

[EDIT] The only way to make it work is via setting <Router key={Math.random()} />

This does not make it work. You are forcing the root component to remount, thereby losing all the local state. If this works well for you, you don’t need React Hot Loader at all because its only purpose is to preserve the local state.

@tomitrescak

This comment has been minimized.

Copy link

tomitrescak commented May 19, 2016

Thanks @gaearon , I already got accustomed to my red friend in the console;). Just for the peace f mind it would be great to have warning displayed as warning, not error, but that's no biggie. I'll wait for RR fix. Thanks for your answer!

@leecade

This comment has been minimized.

Copy link

leecade commented Jun 13, 2016

👍

@chase

This comment has been minimized.

Copy link

chase commented Jul 29, 2016

EDIT: After pulling my repo and doing a clean run, I cannot reproduce the effect unfortunately.

Perhaps I'm missing something, but I seem to have things working without using @jaredpalmer's key={Math.random()} hack and I receive no warnings.

I'm using:

    "react": "^15.0.1",
    "react-dom": "^15.0.1",
    "react-hot-loader": "^3.0.0-beta.2",
    "react-router": "^2.6.0",
    "webpack": "^2.1.0-beta.20",
    "webpack-dev-server": "^2.1.0-beta.0"

The only change I made was instead of using a random key, I set the router's key to a static string.

Basically as you see here: https://github.com/jaredpalmer/react-production-starter/blob/master/client/index.js#L39
Just replace it with <Router routes={routes} history={browserHistory} key="ROUTER" />

Mind you, I'm using async routes using Webpack 2's native module dynamic loading with System.import for most paths.

I'll post my .babelrc and a simplified Webpack config later for others to reproduce.

@frankleng

This comment has been minimized.

Copy link

frankleng commented Jul 31, 2016

Defining a static key has no effect
What seems to work is moving the routes var to a higher scope and then call setTimeout on the hot reload render.
https://gist.github.com/frankleng/39e5953b78d42fdc070eced38bd2b307

@chase

This comment has been minimized.

Copy link

chase commented Jul 31, 2016

@frankleng You're right. It seems it was just a fluke that I cannot reproduce on a clean setup.

EDIT: @frankleng Trying your method, the routes never change after updating. Does it not warn you when adding or removing a route, then navigating to it using browserHistory?
It also doesn't appear you are using AppContainer in that gist, so if it does work, is local state maintained?

@frankleng

This comment has been minimized.

Copy link

frankleng commented Aug 1, 2016

@chase thanks for catching that. might've missed it when I copy and pasted.
I wrap <AppContainer> around react-redux <Provider>

local state is is maintained and not seeing any warnings at the moment.

@calesce

This comment has been minimized.

Copy link
Collaborator

calesce commented Oct 23, 2016

@jgentes as @gaearon said, that will remount all of your components on every update, so state will be lost.

@jgentes

This comment has been minimized.

Copy link

jgentes commented Oct 23, 2016

thanks @calesce , that's good to know. I was refreshing the page anyway, so it's still an improvement for me.

@schickling

This comment has been minimized.

Copy link

schickling commented Nov 17, 2016

Is this issue still on the roadmap for RR3?

@calesce

This comment has been minimized.

Copy link
Collaborator

calesce commented Nov 17, 2016

@schickling It was tracked in ReactTraining/react-router#2182, but it looks like they want everyone to migrate to 4.0 as a solution, and the maintainers won't be doing major updates on 3.X.

RR 2/3 mostly works with React Hot Loader as it currently stands, AFAIK the only issues are the console warning and getComponent not re-running.

@schickling

This comment has been minimized.

Copy link

schickling commented Nov 17, 2016

Thanks for that clear wrapup @calesce!

@Codelica

This comment has been minimized.

Copy link

Codelica commented Dec 7, 2016

RR 2/3 mostly works with React Hot Loader as it currently stands, AFAIK the only issues are the console warning and getComponent not re-running.

Anyone else having issues with RR3 and hot reloading? Normal entries reload fine but doesn't for me. I set them to the same component just to try things, and only the Route entry would hot reload it. Going to try migrating to RR4, but was curious if I'm alone on that issue.

@janv

This comment has been minimized.

Copy link

janv commented Dec 8, 2016

@Codelica I "solved" it like this, with routes in their own module:

let routes = <Route>
  // All your routes
</Route>

// Any update within the app will bubble up to this file
// We can't create a new <Route> element though, because Router won't accept
// updates to its routes prop.
// So we're passing the previous export from update to update to prevent Router from complaining
// The React components themselves will still update properly
if (module.hot) {
  let oldRoutes = module.hot.data && module.hot.data.routes
  if (oldRoutes) {
    routes = oldRoutes
  }
  module.hot.dispose(function(data){
    data.routes = routes
  })
}

export default routes
@awitherow

This comment has been minimized.

Copy link

awitherow commented Jan 21, 2017

@chase is your method here above #249 (comment) still the recommended approach to handling redux-saga?

@chase

This comment has been minimized.

Copy link

chase commented Jan 23, 2017

@awitherow I wouldn't say it is recommended, as I haven't used it with RR4 and stopped using RHL due to flaky behavior. It would not hurt to try and then post your results, though.

@johnnypez

This comment has been minimized.

Copy link

johnnypez commented Feb 16, 2017

I just wanted to share this fairly dirty workaround for hot-reloading async routes.
Like others, I had initially added a random key to my router which blew away state every time anything hot reloaded.
So I poked around in react-router's Router.js by exposing a global reference to it then and tried recreating the internal transitionManager and router objects after a hot reload. This got things working, so I've wrapped react-router with another component that does this teardown and setup any time the routes prop changes.

I know it's bad to call these lifecycle functions directly but it's working for now, is there any other reason why I shouldn't do it?

hacky_router.jsx

import React, {Component} from 'react'
import {Router, browserHistory} from 'react-router'
export default class HackyRouter extends Component {

  componentDidMount(){
    // componentWillReceiveProps just whines about changing the routes prop so this shuts that up
    this.router.componentWillReceiveProps = function(){}
  }

  componentDidUpdate(prevProps){
    if(prevProps.routes != this.props.routes){
      // tear down and set up router internals again
      this.router.componentWillUnmount()
      this.router.componentWillMount()
    }
  }

  render(){
    return <Router ref={ref=>this.router = ref} history={browserHistory} routes={this.props.routes} />
  }
}

entry.jsx

import React from 'react'
import {render} from 'react-dom'
import HackyRouter from './hacky_router'
import routes from './routes'
import {AppContainer} from 'react-hot-loader'

const root = document.getElementById('root')

render((<AppContainer><HackyRouter routes={routes} /></AppContainer>), root)


if (module.hot) {
  module.hot.accept('./routes', () => {
    // reload the routes file
    let nextRoutes = require('./routes')
    render((<AppContainer><HackyRouter routes={nextRoutes} /></AppContainer>), root)
  })
}

@wkwiatek wkwiatek closed this in e41e117 Feb 28, 2017

@yyankovblc

This comment has been minimized.

Copy link

yyankovblc commented Mar 2, 2017

We still reproduce the issue with react-router and redux, in the console we have WDS] App hot update... dev-server.js?05e4:45 [HMR] Checking for updates on the server... log-apply-result.js?e2ce:20 [HMR] Updated modules: log-apply-result.js?e2ce:22 [HMR] - ./components/Login/LoginForm.tsx log-apply-result.js?e2ce:22 [HMR] - ./components/Login/LoginModal.tsx log-apply-result.js?e2ce:22 [HMR] - ./containers/App/index.tsx log-apply-result.js?e2ce:22 [HMR] - ./containers/AppComponent/index.tsx dev-server.js?05e4:27 [HMR] App is up to date. but browser doesn't re-render anything?

@mbonaci

This comment has been minimized.

Copy link

mbonaci commented Mar 7, 2017

Same here, it says App is up to date but the DOM never changes.
All the latest versions:

  • react-router 3.0.2
  • redux 3.6.0
  • react-redux 5.0.3
  • webpack 2.2.1
  • webpack-dev-server 2.4.1
  • react-hot-loader 3.0.0-beta.6
  • babel-cli 6.23.0
  • babel-core 6.23.1
  • extract-text-webpack-plugin 2.1.0
  • postcss-loader 1.3.3
  • sass-loader 6.0.2
  • style-loader 0.13.2

And configuration from here: https://webpack.js.org/guides/hmr-react/

@esayler esayler referenced this issue Apr 9, 2017

Merged

Fix HMR #41

@thomaseizinger thomaseizinger referenced this issue Aug 24, 2017

Closed

Make HMR work properly #38

0 of 2 tasks complete
@aaronatmycujoo

This comment has been minimized.

Copy link

aaronatmycujoo commented Oct 26, 2017

Has there been any progress on this? Also getting a similar issue where the CSS hot reloads 🙌 however the WDS tells me the app is up to date, but no DOM changes are made.

    "react-router": "^2.6.1",
    "webpack": "3.5.5",
    "webpack-dev-server": "^2.7.1"
@neoziro

This comment has been minimized.

Copy link
Collaborator

neoziro commented Oct 29, 2017

@aaronatmycujoo react-router < 4 will not be supported, if you use it you can setup hot reload stuff in Webpack without React Hot Loader. Your app will be entirely refreshed when a change occurs.

@tgroutars

This comment has been minimized.

Copy link

tgroutars commented Nov 11, 2017

Still having this issue with

    "react": "^15.6.1",
    "react-hot-loader": "^3.1.2",
    "react-redux": "^5.0.6",
    "react-router-dom": "^4.2.2",
    "redux": "^3.7.2",
    "webpack-dev-server": "^2.7.1"

Any update? Why was this issue closed? @gaearon @wkwiatek

@theKashey

This comment has been minimized.

@WangYang-Rex

This comment has been minimized.

Copy link

WangYang-Rex commented Jun 14, 2018

@theKashey I had tried everything.. my modules were rebuilding, but the page wouldn't update. Is there a working example for react-routera and redux somewhere?

@theKashey

This comment has been minimized.

Copy link
Collaborator

theKashey commented Jun 15, 2018

Good example will got help you - it should work out of the box. Better provider your example to play with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.