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

webpack2 + hot-reload: Not works with System.import #303

Closed
aight8 opened this issue May 23, 2016 · 34 comments
Closed

webpack2 + hot-reload: Not works with System.import #303

aight8 opened this issue May 23, 2016 · 34 comments

Comments

@aight8
Copy link

aight8 commented May 23, 2016

The change is not applied. Webpack seems to generate a chunk with a new id with the changes for the chunk when using System.import.

@calesce
Copy link
Collaborator

calesce commented Sep 14, 2016

Hi, I'd like to take a look at this. I have yet to mess around much with Webpack 2. Do you have a (minimal) project that reproduces the issue?

@ctrlplusb
Copy link
Contributor

Hi @aight8 and @calesce

I recently got my boilerplate working properly with System.imports without any RHL workarounds.

Originally I had issues when doing static path System.imports, like so:

System.import('../components/Foo')

RHL would not hot reload them the moment I navigated to another route of my application. As a workaround I used to have to do hard require statements whilst in dev mode:

if (process.env.NODE_ENV === 'development') {
  require('../components/Foo');
}

This was tedious to say the least as my async routes grew.

I then introduced an expression based version of my System.import, for example:

function asyncAppViewResolver(viewName) {
  return (nextState, cb) =>
    System.import('../components/App/views/' + viewName + '/index.js')
      .then(module => cb(null, module.default))
      .catch(err => console.log(err));
}

const routes = (
  <Route path="/" component={App}>
    <IndexRoute getComponent={asyncAppViewResolver('Home')} />
    <Route path="about" getComponent={asyncAppViewResolver('About')} />
  </Route>
);

This is more concise, and strangely enough I no longer need to do any workarounds to get RHL working with this. I suspect the way that webpack wraps these kinds of statements plays nicer with RHL.

You can try it out from my repo: https://github.com/ctrlplusb/react-universally

@calesce
Copy link
Collaborator

calesce commented Oct 2, 2016

Thanks for the example @ctrlplusb, we should definitely put this in our docs.

Do you think the issue with static path System.import is just a Webpack 2 bug?

@birkir
Copy link

birkir commented Oct 11, 2016

This is definitely a bug.

TypeError: Cannot read property 'call' of undefined
    at __webpack_require__ (http://localhost:3000/vendor.js:686:29)
    at fn (http://localhost:3000/vendor.js:111:20)
    at http://localhost:3000/client.js:20574:10

Happening here:

modules[moduleId].call(module.exports, module, module.exports, hotCreateRequire(moduleId));

@calesce
Copy link
Collaborator

calesce commented Oct 11, 2016

@birkir: I'm not sure where that comes from without more context. What version of RHL/Webpack, and do you have a simple repo project? Also, it might not strictly fall under this issue, so please open a new issue if you think it's a problem with React Hot Loader.

@birkir
Copy link

birkir commented Oct 11, 2016

Oh yeah, sorry. ueno-llc/starter-kit-historical#16
I'm still looking into this, may have posted too soon.

I get this error on the first request to the server, once i hot reload (any code) everything starts to work as normally.

@elodszopos
Copy link

Hey @ctrlplusb. Regarding your workaround. I've built out async route splitting / chunking using react router too with Webpack 2 and I got all my containers / routes loading on-demand with System.import.

While in dev mode, I get the following:


process-update.js:100 [HMR] Updated modules:
process-update.js:102 [HMR]  - ./UI/helpers/Paginate.jsx
process-update.js:102 [HMR]  - ./UI/pages/Users/Users.jsx
process-update.js:102 [HMR]  - ./UI/pages/Users/index.jsx
process-update.js:102 [HMR]  - ./UI/pages async ^\.\/.*\/index$
process-update.js:102 [HMR]  - ./UI/routes/index.jsx
process-update.js:102 [HMR]  - ./UI/containers/Root/index.jsx
process-update.js:107 [HMR] App is up to date.

All nice and fine, http://localhost:8080/15.c1b371b1060596a59e4e.hot-update.js also appears in the network tab as a request, containing

webpackHotUpdate(15,{

/***/ 303:
/***/ function(module, exports, __webpack_require__) {
...

as expected. Function gets called, module replacement happensss .... but then it doesn't. The UI never updates, the new component never gets rendered. I tried changing the new components (new meaning the one I'm trying to update) render to also log something on the console. Never gets called. No log, no render.

I will try your workaround now and see if that helps. So if that works.. I can officially confirm this one. Unfortunately.

Maybe I'm not on the right track, but I'm running out of ideas now.

@elodszopos
Copy link

elodszopos commented Oct 11, 2016

Update: using require instead of System.import does update the container with the changes after HMR did what it should. But only upon navigating elsewhere and then back to the container that you updated, so router loads it again for you (containing the changes). Same page updating still does not happen. Possibly manually module.hot.accept them?

That can get out hand real quick though. Even with the leaf and parent route abstraction that I did. You can see below.

function getLeafRoute(route) {
  return (nextState, cb) => {
    if (process.env.NODE_ENV === 'development') {
      return loadRoute(cb)(require(`../pages/${route}/index`));
    } else {
      return System.import(`../pages/${route}/index`).then(loadRoute(cb)).catch(errorLoading);
    }
  };
}

Update:
If I try to hot accept the modules, I get:

[HMR] unexpected require(116) from disposed module 26

@elodszopos
Copy link

elodszopos commented Oct 11, 2016

@ctrlplusb I looked at what I had and compared with yours, and it's basically the same.

function errorLoading(err) {
  if (process.env.NODE_ENV === 'production') {
    // force reload with force get the page and load new chunks
    window.location.reload(true);
  } else {
    console.error('Dynamic page loading failed', err);
  }
}

function loadRoute(cb) {
  return (module) => cb(null, module.default);
}

function getParentRoute(route) {
  return (nextState, cb) => System.import('../containers/' + route + '/index.jsx')
    .then(loadRoute(cb))
    .catch(errorLoading);
}

function getLeafRoute(route) {
  return (nextState, cb) => System.import('../pages/' + route + '/index.jsx')
    .then(loadRoute(cb))
    .catch(errorLoading);
}

Here's a group:

const loginGroup = moduleName => (nextState, cb) => {
  const groupItems = {
    login: System.import('../pages/Login'),
    resetPassword: System.import('../pages/ResetPassword'),
  };

  groupItems[moduleName].then(loadRoute(cb)).catch(errorLoading);
};

And some route definitions:

const appRoutes = {
  component: App,
  path: '/',
  childRoutes: [
    {
      getComponents: loggedOutLayout,
      childRoutes: [
        { path: '/login', getComponent: loginGroup('login') },
        { path: '/reset-password', getComponent: loginGroup('resetPassword') },
      ],
    },
...
    childRoutes: [
            { path: 'accountInfo', getComponent: getLeafRoute('AccountInfo') },
            { path: 'users', getComponent: getLeafRoute('Users') },
...

I cannot get it to work though. HMR does happen, I get the update, but the old component never updates. If anyone can provide some assistance it'd be most welcome.

Also, I can confirm that I isolated the problem and it is System.import. Non-asynchronously loaded routes work perfectly fine.

Thank you!

@Jessidhia
Copy link

Jessidhia commented Oct 12, 2016

Just hit this, and I have the same symptoms/conclusion as @elodszopos. Components belonging to an async chunk will log, to the console, as if they have been updated, but their appearance in the UI doesn't change. Even if the component is something simple like only returning <div>foo</div>, changing it to <div>bar</div> will not update the render if the component is in an async chunk (as cut by System.import), but will if the component comes from a regular hoisted import.

This also happens if the component is imported through a hoisted import if the file that has the import / module.hot.accept is a file loaded through System.import; the rerender will misbehave.

As this is an accept for a sync import (inside an async chunk), code generated by webpack is still the same as usual:

import Component from './module/request'

module.hot.accept('./module/request', acceptCallback)

is compiled as (whitespace / newlines added, identifiers simplified for readability):

/* harmony import */ var __WEBPACK_IMPORTED_MODULE_4__module_request__ =
  __webpack_require__(/*! ./module/request */ "moduleId");

module.hot.accept(/*! ./module/request */ "moduleId",
  function(__WEBPACK_OUTDATED_DEPENDENCIES__) {
    /* harmony import */ __WEBPACK_IMPORTED_MODULE_4__module_request__ =
      __webpack_require__(/*! ./module/request */ "moduleId");
    (acceptCallback)(__WEBPACK_OUTDATED_DEPENDENCIES__);
  }
);

EDIT: to clarify, the acceptCallback is called; but nothing changes when ReactDOM.render runs.

@Jessidhia
Copy link

Jessidhia commented Oct 12, 2016

Correction: hot reloading actually does work, but only in one of the copies of the component. Only last component to be rendered in the first pass gets updated.

Is webpack losing the module.hot.accept callbacks?

EDIT: looks like webpack only keeps the last callback. I changed my code to a method where I keep track of each callback, and execute all of them in the callback actually given to module.hot.accept.

@birkir
Copy link

birkir commented Oct 14, 2016

Just a little update.

I fixed the issue by invalidating the first build from webpack. So to sum up, hot reloading system.imported code would only work on the second time the webpack builds.

I don't know if I'm just using hot reloader the wrong way, but It's very similar to @ctrlplusb solution.

https://github.com/ueno-llc/starter-kit/blob/master/webpack/dev.js#L118

@ctrlplusb
Copy link
Contributor

Hi guys, I will have a look at your configurations to see if I can spot anything different to mine. Btw if you are using react router 4 it seems to play much nicer with RHL. I have a working example in here.

Will report back soon with findings.

@elodszopos
Copy link

Awesome @birkir @ctrlplusb. I'll give it a try with both of the (possible?) solutions to see if they work. Will report back as well. Also, if there's any info that you need from me to further clarify or possibly webpack / route setup, can provide that too.

@wunderg
Copy link

wunderg commented Dec 1, 2016

Did you guys found a solution for the HMR problem? I'm stuck with exactly same thing ...

@critic47
Copy link

critic47 commented Dec 3, 2016

I kinda had the same problem. I'm currently running version 3.0.0-beta.2 with webpack@2.1.0-beta.12
Hot reload works fine even with System.import if I just remove react-hot-loader/patch from my configuration.

It doesn't feel quite right though.

@DenisGorbachev
Copy link

Same problem here...

@dobryanskyy
Copy link

Yep, I have the same isssue

@mqklin
Copy link

mqklin commented Jan 9, 2017

Here is the simplest demo.
The problem is obviously not a webpack 2, nor a import(). Please look at my example, I use webpack 1.14.0 with require.ensure.

Code:

import React, { Component } from 'react';
import NotLazy from './NotLazy';

export default class App extends Component {
  state = {
    Lazy: () => null,
  };
  componentWillMount() {
    setTimeout(() => {
      require.ensure([], () => {
        const Lazy = require('./Lazy');
        this.setState({ Lazy: Lazy.default });
      });
    }, 1500);
  }
  render() {
    const { Lazy } = this.state;
    return (
      <div>
        <NotLazy />
        <Lazy />
      </div>
    );
  }
}

NotLazy is editable, Lazy is not. "react-hot-loader": "^3.0.0-beta.6", "webpack": "^1.14.0", "webpack-dev-middleware": "^1.8.3", "webpack-hot-middleware": "^2.12.2"

@wmertens
Copy link

Just confirming that Webpack 2.2 has the same problem. It's probably in the way RHL handles the update?

@wmertens
Copy link

Worked around it by only using splitting for a prod build. See also https://github.com/luqin/react-router-loader to automate this in your webpack config based on filename/path.

@olslash
Copy link

olslash commented Feb 19, 2017

for me, this was causing HMR to fail in System.imported components, which was being done in react-router routes. The solution was simply require()ing the modules at the top of the file if __DEV__ === true (the var is provided by webpack and code protected by it is eliminated in the prod build). A little duplication but not too bad.

if (__DEV__) {
  // require every module that will be System.import()ed below
  require('./components/NotFound');
}

const notFoundRoute = {
  path: '*',

  getComponent: (nextState, cb) => {
    System.import('./components/NotFound')
      .then(loadRoute(cb))
      .catch(errorLoading);
  }
};

@wmertens
Copy link

wmertens commented Feb 19, 2017 via email

@wkwiatek
Copy link
Collaborator

As System.import became deprecated, I would recommend that you take a look at two webpack guides:

Does anyone have problems with the latests versions of webpack / webpack-dev-server and dynamic import?

@olslash
Copy link

olslash commented Feb 19, 2017

@wkwiatek, thanks for pointing that out. I didn't realize System.import had been deprecated.

I did a global find/replace on System.import( -> import( and removed my __dev__ hack, and while codesplitting still works as expected, HMR does not. Replacing the hack fixes it again.

   "webpack": "2.2.1",
    "webpack-dev-middleware": "1.10.1",
    "webpack-hot-middleware": "2.17.0",
    "react-hot-loader": "3.0.0-beta.6",

@wkwiatek
Copy link
Collaborator

wkwiatek commented Feb 19, 2017

Thanks for reporting @olslash !

The question is whether your problem is connected to just dynamic import or rather asynchronous routes (with React Router). Don't you use React Router here?

If yes -> we have another issue for that: #288

If not -> could you provide some repo with minimal setup that fails?

@olslash
Copy link

olslash commented Feb 20, 2017

Thanks for that link, I am using react router, and doing one of the tricks mentioned in there (forcing the root route to re-render on HMR updates) does fix the problem (poorly, but effectively shows that this is an RR issue).

@mqklin
Copy link

mqklin commented Feb 22, 2017

@wkwiatek why did you close this? Is it fixed now?

@wkwiatek
Copy link
Collaborator

@mqklin RHL v3 works with Webpack 2 final, and dynamic imports (see #303 (comment)). Most of the cases here are due to React Router v3 async loading issues (#288).

If there is something more, let me know, I'll definitely reopen it!

@mqklin
Copy link

mqklin commented Feb 22, 2017

@wkwiatek check this https://github.com/mqklin/hot-loader-lazy-modules-bug webpack 2, RHL 3, the simplest config, no React Router. Doesn't work.

@wkwiatek
Copy link
Collaborator

I just copy-pasted your code into my playground and it worked. However, it's doing full reload (you're changing the state of the root component).

I'll try to take a look later what's causing the issues in your repo. I bet there are some config-related problems (which are probably not covered in docs).

@mqklin
Copy link

mqklin commented Feb 22, 2017

Thank you for the example config. I can't use webpack-dev-server because sometimes I need to mock backend, that's why I use webpack-dev-middleware and webpack-hot-middleware.

@wkwiatek
Copy link
Collaborator

wkwiatek commented Feb 22, 2017

I totally understand, and RHL should work with both so we should investigate it deeper. Would you mind to create an issue for that (as related to webpack middlewares) with your specific example? I just don't want to mess in this thread which is really about webpack 2 and System.import in general.

@mqklin
Copy link

mqklin commented Feb 27, 2017

Please look #490

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

No branches or pull requests