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 HMR running #2

Closed
grabbou opened this issue Mar 23, 2017 · 16 comments
Closed

Get HMR running #2

grabbou opened this issue Mar 23, 2017 · 16 comments
Assignees

Comments

@grabbou
Copy link
Member

grabbou commented Mar 23, 2017

Get HMR up & running. If supported by default, we should make a flag --hot on a CLI. I believe that switch from Dev Tools is rather useless. All in all, you don't want to change it during development.

Eventually we might want to change dev tools in Simulator as well (given that it's been recently allowed), but we will see.

@grabbou grabbou self-assigned this Mar 23, 2017
@grabbou
Copy link
Member Author

grabbou commented Mar 24, 2017

Not a top priority to me for now as I never used HMR in React Native, partially because it's default implementation is rather poor to me.

We might want to implement it after going alpha.

Also, might be a good first task for outside collaborators, should be fairly easy to do with a standard Webpack setup.

@Kerumen
Copy link
Contributor

Kerumen commented Mar 30, 2017

it's default implementation is rather poor to me

Can you elaborate? I find it very useful and it works well. If you are in a deep nested screen of your app, you don't have to retraverse the whole app each time you edit a fontSize.

@SEAPUNK
Copy link
Contributor

SEAPUNK commented Mar 30, 2017

Will implementing HMR via react-hot-loader 3 work?

I attempted to get it working locally once (we glued react-native's packager and webpack together via react-native-webpack-server), but the only problem I had was making the webpack dev client work, since the code ran in a worker context, and that had some weird issues.

@grabbou
Copy link
Member Author

grabbou commented Mar 31, 2017

@SEAPUNK technically it should, feel free to give it a go and share your haul config!

@Kerumen it works, but I was referring to this part specifically facebook/react-native#10991 (comment)

@migueloller
Copy link
Contributor

I tried setting up react-hot-loader v3. The only issue I ran into was the inability to do something like ReactDOM.render on React Native. Doesn't look like AppRegistry offers something similar.

Any ideas?

@satya164
Copy link
Member

Probably just a this.forceUpdate() will work?

@satya164
Copy link
Member

Or something like,

export default class App extends Component {
  state = { root: App };

  componentWillMount() {
    if (module.hot) {
      module.hot.accept('./App.js', () =>
        this.setState({ root: require('./App.js').default })
      );
    }
  }

  render() {
    return React.createElement(this.state.root); 
  }
}

@migueloller
Copy link
Contributor

migueloller commented Apr 20, 2017

@satya164, yup! I realized this soon after the comment. 😅

The issue I'm running into now is that webpack-dev-middleware doesn't come with HMR support out of the box. Based on Webpack 2 docs, the hot options isn't compatible with webpack-dev-middleware.

See here.

@satya164
Copy link
Member

satya164 commented Apr 20, 2017

Yes, we'll need to use webpack-hot-middleware - https://github.com/glenjamin/webpack-hot-middleware

@migueloller
Copy link
Contributor

migueloller commented Apr 21, 2017

I played around with webpack-hot-middleware for a bit. It doesn't work quite nicely out of the box.

I was able to get HMR set up but for some reason requests to the manifests were timing out. Check out this PR.

Also, to get it working without having to use remote JS debugging, we'll have to add a polyfill for window.EventSource to satisfy this.

EDIT: It looks like a custom HMR middleware will be needed.

@zamotany
Copy link
Contributor

zamotany commented Jun 9, 2017

I checked metro-bundler code and found that react-native adds HMR handler which probably sends HRM bundle as a JSON string via WebSocket: https://github.com/facebook/react-native/blob/328d8ddb4781dc270d065aa45033b6be565b2c8f/local-cli/server/util/attachHMRServer.js#L202-L389

In this case we would definitely need a custom HMR middleware.

@zamotany
Copy link
Contributor

zamotany commented Jun 14, 2017

As you can see in https://github.com/facebook/react-native/blob/master/Libraries/Utilities/HMRClient.js#L32-L137, when HMR is enabled by clicking Enable Hot Reloading RN app will connect to WebSocket under path /hot, whenever change in source file is detected Haul's custom HMR middleware should send JSON.stringify({ type: 'update-start' }) message via this websocket, then create body of HMR update, send it and then update-done. The source of how this all is done is here and as you can see it calls to few functions from metro-bundler: https://github.com/facebook/metro-bundler/blob/master/packages/metro-bundler/src/Server/index.js#L311-L361

We don't need any react-hot-loader since it's bundled with RN staring from version 0.22 or sth like that. You can read about implementation in this article: https://facebook.github.io/react-native/blog/2016/03/24/introducing-hot-reloading.html

So in order to support HMR we need to write custom middleware that will create a bridge between webpack and HMR client. As in the mentioned article, implementation is similar to Webpack HMR API so it should be a matter of copy/pasting and tweaking it a little bit.

You can track progress on this matter here: https://github.com/callstack-io/haul/compare/feature/hmr

@zamotany
Copy link
Contributor

zamotany commented Oct 8, 2017

Done in #182

@zamotany zamotany closed this as completed Oct 8, 2017
@grabbou
Copy link
Member Author

grabbou commented Oct 8, 2017

@alloy Do you have any feedback regarding initial HMR setup that you made? I heard (not sure it was you) that it's non trivial. Is there anything we can do in order to improve it?

@orta
Copy link
Contributor

orta commented Oct 9, 2017

I struggled to get it working with Storybooks, which is our main development environment and eventually ran out of my timebox'd time. Our RN app exposes many AppRegistry.registerComponent so I needed to handle it manually and I couldn't figure out how to make the root exposed module be one that comes from a module.

@grabbou
Copy link
Member Author

grabbou commented Oct 11, 2017

CC: @zamotany who authored this module. Guys, let's sync on Slack and figure it out.

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

7 participants