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

Hot Reload doesn't work when starting inside a function #765

Closed
chrisabrams opened this issue Jan 5, 2018 · 18 comments
Closed

Hot Reload doesn't work when starting inside a function #765

chrisabrams opened this issue Jan 5, 2018 · 18 comments
Labels

Comments

@chrisabrams
Copy link

If you are reporting a bug or having an issue setting up React Hot Loader, please fill in below. For feature requests, feel free to remove this template entirely.

Description

For the next branch, the hot reload does not work if the component being rendered is started inside a function.

Expected behavior

Being inside a function should not make any difference.

Actual behavior

It only works if the component being rendered is run at the top level. Meaning, the top level file that calls ReactDOM.render has to run directly, ReactDOM.render cannot be run inside a function that mounts the component which has been "hotted."

Environment

React Hot Loader version: next, ^4.0.0-beta.12

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

  1. node -v: v8.9.0
  2. npm -v: 5.5.1

Then, specify:

  1. Operating system: OS X 10.12.4
  2. Browser and version: Chrome 63

Reproducible Demo

Doesn't work

https://gist.github.com/chrisabrams/e0fe9cf8ea7347fe2364cc46cf378412

Does work

https://gist.github.com/chrisabrams/293a08cfb98b6d8f0bb5aff4464f5874

@theKashey
Copy link
Collaborator

Yet again. You shall mark as HOT EXPORTS of a module. Not imports.
Never place hot in the same module, you perform a render.

So - just extract App into separate file, end export default hot(module)(App).

@gregberge
Copy link
Collaborator

@theKashey we have to detect it, too many bugs are caused by wrong configuration.

theKashey added a commit to theKashey/react-hot-loader that referenced this issue Jan 5, 2018
@theKashey
Copy link
Collaborator

PS: The both examples are not working. The first just failing with js Error, but the second is failing to preserve the state.

@chrisabrams
Copy link
Author

chrisabrams commented Jan 5, 2018

@theKashey What JS error? First example works just fine for me. The issue I have is that I don't know what the component is going to be, so I need to put it inside a function. I can't export default it directly as it might be wrapped by another component. I was hoping that I could do something such as:

export default function renderHot(Component) {

    return hot(module)(Component)

}

which would make it hot on the fly. That does not work though. To be more clear:

This works:

import React from 'react'
import { hot } from 'react-hot-loader'

const AppC = () => <div>Hello World!! 10</div>
const HotApp = hot(module)(AppC)

export default function App() {

  return HotApp

}

This does not work:

import React from 'react'
import { hot } from 'react-hot-loader'

const AppC = () => <div>Hello World!! 10</div>

export default function App() {

  return hot(module)(AppC)

}

What specifically makes hot being called inside a function cause [HMR] The following modules couldn't be hot updated: (Full reload needed)?

@chrisabrams
Copy link
Author

chrisabrams commented Jan 5, 2018

Maybe I should rephrase my question..how would I correctly place a component inside another component for hot reload, but I don't know what the inner component is going to be? The below code does not hot reload, but is a basic idea of what I am trying to achieve. I don't know what component is going to be rendered until run-time.

import React from 'react'
import { hot } from 'react-hot-loader'

export default function App(Component) {

  const Layout = () => <div>{Component}</div>
  return hot(module)(Layout)

}

@theKashey
Copy link
Collaborator

You saw [HMR] The following modules couldn't be hot updated: (Full reload needed) cos there was JS error on render.

The only correct way to use hot is:

import React from 'react'
import { hot } from 'react-hot-loader'

export default hot(module)(function App(Component) {

  const Layout = () => <div>{Component}</div>
  return Layout
})

it should be applied diffectly to the export.
If you need some complex logic, and just dont have a Component as a top level variable/export – then create it, and put everything inside it.
There is no need to mark all components as hot, - if you will mark only top-level application - it will be enought.

import React from 'react'
import { hot } from 'react-hot-loader'
import createLayout from './createLayout'

const Something = createLayout(SomeAnotherComponent);
export default hot(module)(function App  {
  return <Something />
}

@gregberge gregberge added question and removed bug labels Jan 6, 2018
gregberge pushed a commit that referenced this issue Jan 6, 2018
Idea - detect the module execution start and the module execution end.
During this event HotExportedComponent could be mounted and rendered, but shall not be unmounted.

If this event will occur - thus means that react-dom's render method was called, and that is misconfiguration case we are trying to solve.

Fixes #765
@gregberge
Copy link
Collaborator

@chrisabrams thanks for reporting, we added a warning that should help detecting wrong usage. I close this issue, feel free to ping us or to reopen a new one if you still have some problems using React Hot Loader.

@chrisabrams
Copy link
Author

Thanks @neoziro, @theKashey I tried to follow the example provided but I cannot get the hot module function exported to return a component function/class. React continues to complain that it is returning an object :O

I have created a sample repo with three files outlining what I am trying to do: https://github.com/chrisabrams/sandbox-hot-reload

The src directory contains the three files.

@chrisabrams
Copy link
Author

I tried placing a console.log inside the function that is passed into hot(module) but the console.log did not fire. Now I'm trying to figure out if that function is being executed or something else is being returned.

@chrisabrams
Copy link
Author

chrisabrams commented Jan 6, 2018

When I check the typeof WrappedComponent here it is a function, what would cause React to then error and say that it's actually an object in the app here? All the function is doing is returning whatever hot module returned.

@chrisabrams
Copy link
Author

The challenge for me is that hot(module)(func) returns a class called ExportedComponent which requires new which then causes the object to be returned. If I try to use the return of the hot module export without new then the console complains that ExportedComponent requires new even if I'm trying to inject it into React directly.

@chrisabrams
Copy link
Author

To sum up my comments:

Why does this error with Uncaught Error: Objects are not valid as a React child (found: object with keys {props, context, refs, updater}). If you meant to render a collection of children, use an array instead.?

export default hot(module)(function renderHot(Component) {

  const Wrapped = () => Component

  return Wrapped

})

But this works?

export default function renderCold(Component) {

  const Wrapped = () => Component

  return Wrapped

}

@theKashey
Copy link
Collaborator

Cos you can not wrap with hot a function, HOC in your case.
You have to wrap Component!
And we cant detect the problem here, as long any function could be a component

@theKashey
Copy link
Collaborator

In you case you can wrap theme with hot, and may be it will the behavior you are looking for.
But! There is an easier way to do all the stuff - "the old way"

mport React from 'react'
import ReactDOM from 'react-dom'
import {AppContainer} from 'react-hot-loader'
import renderHot from './renderHot'
import Theme from './theme'

function render(){
const Component = new renderHot(<Theme />)
console.log('type theme', typeof Theme)
console.log('type hot', typeof Component) // Why am I type object when a function should be returned?
// Wrap everything with AppContainer
ReactDOM.render(<AppContainer><Component /></AppContainer>, document.getElementById('root'))
}

render();

if(module.hot) {
  // accept the changes from that modules and re-render the app
  module.hot.accept('./renderHot', render);
  module.hot.accept('./theme', render);
}

Dont forget to set modules:false in babel.rc

This is actually the way v3(current) version work, explained as advanced usage in v4.
It will perfectly fit your requirements.

@chrisabrams
Copy link
Author

I followed your first example exactly so I'm not sure why you are saying I can't do that anymore. I just want to pass a component inside the function, and have that function return a component which is hot.

I don't think that the v3/advanced way will work because I don't know what component I'll be wrapping, aka, I can't supply that component path in the if(module.hot) statement because I don't know where the component will be coming from.

@theKashey
Copy link
Collaborator

The last example will work, as long it will work on application top-most level. There is nothing higher.
The example with hot will not work, as long hot was created to wrap a single Component. Let me explain the word "simple":

export default hot(module)(function App(props)  {  <---- this is simple stateless functional component
  return <Something />
}

export default hot(module)(function App(Component)  { <-- this is not
  return <Component />
}

This is a way to make it work

const makeItHot = hot(module); // get `the hot`, and mark module as hot 
export default function renderHot(Component) {
  const Wrapped = () => Component
  return makeItHot(Wrapped) <-- apply `hot` here.
})

This will work, but will completely ignore different Components you may provide here – for any instance it always be "the last used".
The second problem, you have to understand, - it will react only to the changes made in this, or dependent files. Not inside the Component.
To make it work - you have to wrap Theme with hot.

But better do as I said - dont wrap anything with hot, use a bit more low-level API to wrap the topmost wrappable place in the entire application.

I don't think that the v3/advanced way will work because I don't know what component I'll be wrapping, aka, I can't supply that component path in the if(module.hot) statement because I don't know where the component will be coming from.
Currently, this is only 2 possible files. List of dependencies is limited. In common applications - the is just the single file - "App.js". As long all the changes made used dependencies will bubble thought parents - you can accept only the App.js and that's done.

If you still don't clearly understand how it work - please invest some time into experiments. Try just to rethink the way you stuck with.

@chrisabrams
Copy link
Author

Thanks. What I'm working on, I don't really know what the top most level component will be. I can create a wrapping component, but then that brings back to the issue you are describing where I would still need to wrap the second level component, which I won't know. I was able to get it working by wrapping the theme itself, that's good enough for now. Thanks for your help I'm sure I'll comment again when I come across a more advanced case.

@humam-nameer-10p
Copy link

Any example of how to make it work with redux connect() ?


Example:
export default connect(mapStateToProps)(App)

Only App.js and Index.js is hot-reloading rest doesnot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants