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

Support React 16.3 #918

Closed
theKashey opened this issue Mar 31, 2018 · 46 comments
Closed

Support React 16.3 #918

theKashey opened this issue Mar 31, 2018 · 46 comments
Assignees

Comments

@theKashey
Copy link
Collaborator

Currently, we rely on componentWillReceiveProps to perform sync update and sync forceUpdate. So - this method does not exist anymore.

We have 2 options:
1 - rely on subrender, ie async updates. It is a dev mode, nobody cares about double rendering on rare hot updates. Thus remove AppContainer completely. This will also solve
2 - find another way. But I don't see any.

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Mar 31, 2018

Would it also explain why I see a warning anytime is use getDerivedStateFromProps()?
I'm assuming react-hot-loader is hijacking the components and add componentWillReceiveProps .

capture d ecran 2018-03-31 a 13 20 33

@gregberge
Copy link
Collaborator

Yes, new challenge to come!

@theKashey
Copy link
Collaborator Author

++ we have to test how new ("real") Context API works, as long "Context" is not a "Tag" or a function - it is a brand new element type. Should be fine, but even enzyme itself failing :)

@theKashey
Copy link
Collaborator Author

@oliviertassinari - yeah, RHL is adding componentWillReceiveProps, and I am going just to drop it.

@gregberge gregberge changed the title No more componentWillReceiveProps [React 16.3] Support React 16.3 Apr 1, 2018
@swashata
Copy link

swashata commented Apr 2, 2018

Had just this issue right now. I am very new to react, just started and had to scratch my head (like a lot) to understand this isn't coming from mine, rather the hot loader 😁 . I will be waiting for an update.

@gaearon
Copy link
Owner

gaearon commented Apr 2, 2018

I don't understand what this means:

rely on subrender, ie async updates.

@theKashey
Copy link
Collaborator Author

@gaearon - this is a local meme( #830 ), about performing hot-render each time you "can do it". V3 uses AppContainer to start an update, v4 can use AppContainer to perform sync updates, and Component.render to perform async updates. Async cos they could be executed after the main update or main update could not "render into them" - code splitting, resource load, portals, and so on. Plus we have to defer .forceUpdate, as long shall not call it from a render method.
Basically - this one line - https://github.com/gaearon/react-hot-loader/blob/master/src/proxy/createClassProxy.js#L164

@gaearon
Copy link
Owner

gaearon commented Apr 2, 2018

What are you using componentWillReceiveProps for today?

@theKashey
Copy link
Collaborator Author

theKashey commented Apr 2, 2018

Nothing but to being able "flush"(forceUpdate) changes. As I pointed above - we could safely remove it. It may cause double renders, but this is not something we should avoid.
Plus, if you move updates from a timeout to componentDidUpdate - they will be not so very deferred.

@gaearon
Copy link
Owner

gaearon commented Apr 2, 2018

Got it. componentWillReceiveProps is a bad place to call forceUpdate so that sounds good.

@theKashey
Copy link
Collaborator Author

PS: Is there any "new"(~fiber) way to trigger re-render? Is there any way to re-render SFC without a real Component on top of it? Is there any a bit more "soft" ways to update?

@gaearon
Copy link
Owner

gaearon commented Apr 2, 2018

Can you be more specific? Maybe a small code example?

@theKashey
Copy link
Collaborator Author

  • We found component to be updated.
  • We updated it.
  • Now we have to redraw it.

Do we have other choice rather then calling .forceUpdate? How do new Context API handle it?
Actually, what we are trying to do every time - is to update everything from some point and beyond. Just everything will also be a good solution.

@gaearon
Copy link
Owner

gaearon commented Apr 3, 2018

Isn't that what react-force-deep-update already doing? Do you need something else?

@theKashey
Copy link
Collaborator Author

Yep, this is related to react-force-deep-update. I don't really need anything else, but have to ask you about possibilities.

@gaearon
Copy link
Owner

gaearon commented Apr 3, 2018

React maintains a tree of nodes per root. Each node in a tree has an "expiration time" which can either be some time in the future, a marker that says "nothing to do here" or a marker that says "flush it synchronously".

When we forceUpdate or setState, what really happens is:

  1. We also push its new state value into node's "update queue" (or set a flag for forced update)
  2. We mark the related node as having "expiration time" of "synchronous" (unless in async mode)
  3. We mark all its parent nodes up to the root the same way
  4. We schedule React to "work" on this root

React loops over all scheduled roots. That list contains this root (because we just "scheduled" it). React then looks at all its child nodes that have expiration time that is not "nothing to do here", depth first.

That's why it goes over each ancestor. It "skips over" the deep parents (before setState / forceUpdate) because they don't have anything new in the update queue and therefore they're safe to not re-render. But it still looks at their children because it knows something inside called setState / forceUpdate and needs to re-render. Potentially more than one thing.

Eventually it gets to the child that actually has something new in the update queue (our node) and that's when it starts re-rendering.

I hope this gives some insight into how it works internally. We could expose some of this, or we could keep using hacky internal APIs, or a mix of both. The question is what would be valuable to expose. I don't remember enough about how RHL works to say what would be, but maybe you could tell me?

Does this help at all?

@theKashey
Copy link
Collaborator Author

Look like we still have to use forceUpdate to mark node to be updated. We could, but shouldn't use setState for this, as long it may alter the way Component work.
And this is a less hacky way for now.

I don't remember enough about how RHL works to say what would be, but maybe you could tell me?

  • On HMR we call .forceUpdate on AppContainer bound to updated branch.
  • Everything starts on update, in the render method after this issue being solved.
  • RHL checks that "this.version" is the same as "current" value(number of reactHotLoader.register calls), and if not - trigger hot-replacement-render.
  • hot-render hydrates the tree and next start rendering component, comparing to hydrated or registered components, performing proxies update then possible, and calling componentWillUpdate(to bypass redux component cache)
  • everything ends with another (forced) update, to actually display all the changes to the customer, bypassing PureComponents.

Ie - double, but in real triple, render. As long extra time, it will consume, is not comparable to the rebuild time - this is fine.

PS: And thank you for a conversation, I've just understood that comparing this.version and "current" is the root cause of #923, as long current does not increment without babel plugin.

@jaydenseric
Copy link

jaydenseric commented Apr 13, 2018

@gaearon can this please be elevated from an "enhancement" to a "bug"? Current stable React is meant to be supported according to peer deps, yet here is the sort of console output we have to ignore:

screen shot 2018-04-13 at 12 57 34 pm

These false positives are a big confusion and obfuscate real warnings that are really helpful in this period of learning and transition to the new component API. This issue will affect the DX for a lot of people; react-hot-loader has nearly half a million installs/wk and is a dependency of Next.js.

Is there something blocking #927?

@theKashey
Copy link
Collaborator Author

@jaydenseric - nothing. Did not get a chance to properly test the changes. Should be ok, but I am not going to ship something, which could possibly break your stuff.
You can help - just checkout that branch, build RHL and replace node_modules/react-hot-loader/dist by the new one (do not use yarn link, as long RHL will patch its own version of React in that case)
Gonna test the changes one more time and merge the PR. Aiming to ship on weekends.

@theKashey theKashey self-assigned this Apr 13, 2018
@theKashey theKashey added the WIP label Apr 13, 2018
theKashey added a commit that referenced this issue Apr 18, 2018
Making RHL more React 16.3 compatible #918
@gaearon
Copy link
Owner

gaearon commented Apr 19, 2018

I think the issue is that React uses a flag on the method to determine whether it's coming from react-lifecycles-compat polyfill (in which case the method shouldn't lead to a warning). But proxying doesn't preserve the flag.

If I change this method to include

    Object.assign(wrappedMethod, realMethod);

before returning wrappedMethod then the issue goes away.

@theKashey
Copy link
Collaborator Author

I'll update proxy code to keep everything from original class. @gaearon - should I also keep method arity, or (I hope) argument count does not count?

@gaearon
Copy link
Owner

gaearon commented Apr 19, 2018

Arity shouldn’t matter although preserving it is always nice if it’s not too difficult.

theKashey added a commit that referenced this issue Apr 20, 2018
This includes name, arity, toString and everything else possible stored among original method.
@theKashey
Copy link
Collaborator Author

In review.

theKashey added a commit that referenced this issue Apr 20, 2018
This includes name, arity, toString and everything else possible stored among original method.
theKashey added a commit that referenced this issue Apr 20, 2018
This includes name, arity, toString and everything else possible stored among original method.
theKashey added a commit that referenced this issue Apr 20, 2018
fix: Proxy should keep methods own props. #918
@theKashey
Copy link
Collaborator Author

Patch version released. v4.1.1

@elisherer
Copy link

I'm working with React 16.3 and the HMR is broken in 4.1.0 and 4.1.1.
Changing even a simple string inside a div won't appear in the browser after module updates (in my case a function component, didn't check a class one).
rolling back to RHL 4.0.1 brings back the HMR.

@gaearon
Copy link
Owner

gaearon commented Apr 20, 2018

Can you provide a repro case?

@elisherer
Copy link

You caught me unprepared, it might take me a lot of time. My project is not small, but it doesn't use anything special either.
Some code I use:

package.json

  "scripts": {
    "start": "webpack-serve",

.babelrc

{
  "presets": [
    "@babel/preset-flow",
    "@babel/preset-react",
    ["@babel/preset-env", { "modules": false }]
  ],
  "plugins": [
    "@babel/plugin-proposal-class-properties",
    "@babel/plugin-proposal-object-rest-spread",
    "@babel/plugin-syntax-dynamic-import"
  ],
  "env": {
    "test": {
      "plugins": [
        "@babel/plugin-transform-modules-commonjs"
      ]
    },
    "development": {
      "plugins": [
        "react-hot-loader/babel"
      ]
    },
    "production": {
      "plugins": [
        "@babel/plugin-transform-react-constant-elements",
        "@babel/plugin-transform-react-inline-elements",
        "babel-plugin-transform-react-pure-class-to-function",
        "babel-plugin-transform-react-remove-prop-types"
      ]
    }
  }
}

App.js

...

let exportedApp = App;

if (__DEV__) {
  const { hot }  = require('react-hot-loader');
  exportedApp = hot(module)(App);
}

export default connect(mapStateToProps)(exportedApp);

index.js

import React from 'react';
import { render } from 'react-dom';
import App from './components/App';

import { Provider } from './store'; // store made with the new Context API

const appElement = document.getElementById('root');

render(
  <Provider>
    <App />
  </Provider>
, appElement);

@theKashey
Copy link
Collaborator Author

@elisherer - you can activate debug mode - https://github.com/gaearon/react-hot-loader#setconfigconfig - and it shall explain what is wrong.

@theKashey
Copy link
Collaborator Author

I've double checked and added a simple test to be confident about - everything is still 👌

@elisherer
Copy link

@theKashey, Sorry, I don't get any debug info besides the regular logs:
image

@theKashey
Copy link
Collaborator Author

@elisherer - just to double check - App is included into templates/index or templates/index is included in App?

The main difference between 4.0.1 and 4.1 - SFCs are no longer converted into Stateful components, and as result - they lose "brains".
They could not found that "O! I was updated! Hot reloading", and have to had AppContainer(or hot), or any other "smart" Stateful component before them.

Your application worked because "safety net" worked. I've removed it from SFC, and it got broken.
I am not sure how you did it. And I could not reproduce it.

I'll be quite keen to get an example to play with. You can send it in private, if it is not public.

Also, you can try to restore that net - just add App.contexTypes = {}. SFCs with context are still have to be converted into Stateful ones with safety things enabled.

That safety net actually activates "React-Hot-Loader" if it did not properly do it's work. As long there are no errors - look like RHL is just doing nothing, ignoring the update.

@swashata
Copy link

Here's my two cents. I have followed the instruction and it is just working good with React 16.3.

Foo.jsx

import React from 'react';
import { hot } from 'react-hot-loader';
import PropTypes from 'prop-types';

class Foo extends React.PureComponent {
	static propTypes = {
		categories: PropTypes.arrayOf(PropTypes.number),
	};

	static defaultProps = {
		categories: [],
	};

	state = {
		foo: 'Bar',
	};
	render() {
		return (
			<div className="foo-container">{this.state.foo}</div>
		);
	}
}

export default hot(module)(Foo);

The only difference is that I haven't made an export hot, while connecting it with the store. I am using Redux though and my App.jsx looks different.

App.jsx

import React from 'react';
import { render } from 'react-dom';
import { Provider } from 'react-redux';

import Foo from './Foo';
import store from './store';

const Root = (
	<Provider store={store}>
		<Foo />
	</Provider>
);

render(Root, document.getElementById('foo-app'));

And to have hot reloading within the store,

store.js

import { createStore } from 'redux';
import rootReducers from './reducers/rootReducers';

// Create the store
const store = createStore(
	rootReducers,
	/* eslint-disable no-underscore-dangle */
	// Add devtool in an isomorphic way
	typeof window !== 'undefined' &&
		window.__REDUX_DEVTOOLS_EXTENSION__ &&
		window.__REDUX_DEVTOOLS_EXTENSION__()
	/* eslint-enable */
);

// webpack hot middleware support
if (module.hot) {
	module.hot.accept('./reducers/rootReducers', () => {
		/* eslint-disable global-require */
		const nextRootReducers = require('./reducers/rootReducers').default;
		/* eslint-enable */
		store.replaceReducer(nextRootReducers);
	});
}

// export the store
export default store;

And all these setup works with webpack dev middle and webpack hot middleware. Since I am developing a WP plugin, I couldn't use Webpack Dev server directly. So I used the middlewares inside browsersync (all connected with a simple gulp task).

gulpfile.js

const gulp = require('gulp');
const browserSync = require('browser-sync').create();
const webpack = require('webpack');

const webpackDevMiddleware = require('webpack-dev-middleware');
const webpackHotMiddleware = require('webpack-hot-middleware');

// Get the function for creating varying config
const webpackConfig = require('./config/webpack.config');
// Our local config
const localConfig = require('./config/local.config');

gulp.task('serve', () => {
	const config = webpackConfig('development', localConfig);
	const webpackBundler = webpack(config);
	const devMiddleware = webpackDevMiddleware(webpackBundler, {
		noInfo: true,
		publicPath: config.output.publicPath,
		stats: { colors: true },
	});
	const devHotMiddleWare = webpackHotMiddleware(webpackBundler);
	browserSync.init({
		logLevel: 'info',
		port: localConfig.port,
		ui: localConfig.ui,
		debugInfo: true,
		proxy: {
			target: localConfig.proxy,
			// Middleware for webpack hot reload
			middleware: [
				// converts browsersync into a webpack-dev-server
				devMiddleware,
				// hot reload JS
				devHotMiddleWare,
			],
		},
		host: localConfig.host,
		open: false,
		notify: localConfig.notify,
		background: true,
	});

	// Full reload on php changes
	gulp.watch(PATHS.php.src).on('change', browserSync.reload);
	// JS & CSS will be handled by webpack on the go
});

The codes are stripped down so those are missing some imports.

I hope the concepts and codes above helps @elisherer to debug your issue. I don't think it has anything to do with react-hot-loader rather how you have implemented it.

Also, I guess you do have the "react-hot-loader/babel" plugin in your .babelrc file?

@swrobel
Copy link

swrobel commented Apr 24, 2018

@theKashey this is still open despite being listed as fixed in the 4.1.0 release notes. Is there a commit that fixed it?

@theKashey
Copy link
Collaborator Author

theKashey commented Apr 24, 2018

@swrobel it was "fixed" in 4.1.0, and yet again in "4.1.1" 4 days ago, and now I am waiting confirmation from @elisherer and @mvestergaard that this time everything is ok.

All the tests I've done myself - green.

@elisherer
Copy link

ill try to create a repro repo asap

@mvestergaard
Copy link

@theKashey my issue was resolved with 4.1.1, thank you.

@elisherer
Copy link

elisherer commented Apr 24, 2018

It has something to do with me using react-waterfall which isn't doing anything wrong (from my perspective).
Check it out on: https://github.com/elisherer/react-hot-loader-repro918
I added examples of:

  • Class component
  • Function component
  • Pure Class component
  • Consumer Class component
  • Consumer Function component
  • Consumer Pure Class component
  • Children as a Function component
  • Consumer "Connected" component
  • Connected Children as a Function component
  • Component in a portal (modal)

All is working well besides the react-waterfall example.
react-waterfall is a mixture of the above and still manages to break, I added the library's source into the repo so you could fiddle with it.
I Couldn't get it to work.

Maybe I should open another issue, since it doesn't relate to @mvestergaard's issue.

@theKashey
Copy link
Collaborator Author

@elisherer - this is not due to this issue, and it is easy to "fix" it - just remove "Prevent" component.
That means - "forceUpdating" of PureComponents might be broken. That's another issue.

This one is ✅🤘

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

9 participants