Sub render #830

theKashey · 2018-01-27T12:44:19Z

This implements idea behind #806 - each Component become a AppContainer.

How - just via pre-render hook it will pre-hot-render tree as AppContainer can do.
Difference - it will do it on Component render, and force-update afterwards. AppContainer is still needed(preferred) to force sync update.

Solves:

async components will reconcile them self. No need to mark components as hot - they are all hot
solves - a bit of Portals. I am not sure why, but this is not working at 100%. In the example async portal is OK, while sync portal will reset Counter if you will change Counter. And not reset if you will change "hidden counter", which got unmounted in the first case.

Anyway - this feature brings stability, and could solve close to any render quiz, we can get. I am not touching hot-render here, but it could be heavy simplified due to multiple "reconcile" points.

gregberge

The example needs cleaning, I think we need more test on this.

gregberge · 2018-01-29T22:10:08Z

examples/async-portals/package.json

+    "webpack-dev-server": "^2.9.7"
+  },
+  "dependencies": {
+    "emotion": "^8.0.12",


I think we do not need emotion in this project.

gregberge · 2018-01-29T22:10:54Z

examples/async-portals/src/Counter.js

+  render() {
+    return (
+      <span>
+        +{this.state.count}:{this.gen++}


Examples should reflect real examples, I think this is not a real example. I suggest to create a simple Portal example.

Or an Async Portal but without tricky things.

I am using them for debug purposes. They reflect only the pure idea of RHL - don't lose the state.
This example is a good one, cos it actually not working for sync portal, as described above.

gregberge · 2018-01-29T22:11:53Z

examples/async-portals/webpack.config.babel.js

+  module: {
+    rules: [
+      {
+        //exclude: /node_modules|packages/,  // should work without exclude


Excluding node_modules is a good thing, please add it and remove comment.

As long CRA will not exclude node_modules and some projects might follow the same approach - we shall be ready (and we are ready).

gregberge · 2018-01-29T22:13:45Z

examples/async-portals/src/async-component.js

+class AsyncComponent extends Component {
+  constructor(props) {
+    super(props)
+    this.state = {}


We have class properties 😁.

gregberge · 2018-01-29T22:15:13Z

packages/react-hot-loader/src/reconciler/proxyAdapter.js

+  return element
+}
+
+setStandInOptions({


We should definitely merge projects.

gregberge · 2018-01-29T22:15:24Z

packages/react-hot-loader/test/hot.dev.test.js

@@ -63,7 +64,7 @@ describe('hot (dev)', () => {
    wrapper.unmount()
  })

-  it('should redraw component on HRM', done => {
+  it.only('should redraw component on HRM', done => {


codecov-io · 2018-01-30T07:06:57Z

Codecov Report

Merging #830 into next will increase coverage by 1.2%.
The diff coverage is 94.28%.

@@           Coverage Diff           @@
##           next     #830     +/-   ##
=======================================
+ Coverage    85%   86.21%   +1.2%     
=======================================
  Files        23       25      +2     
  Lines       507      573     +66     
  Branches     96      151     +55     
=======================================
+ Hits        431      494     +63     
- Misses       61       62      +1     
- Partials     15       17      +2

Impacted Files	Coverage Δ
packages/react-hot-loader/src/reactHotLoader.js	`100% <ø> (ø)`	⬆️
packages/react-hot-loader/src/utils.dev.js	`100% <ø> (ø)`	⬆️
...ackages/react-hot-loader/src/reconciler/proxies.js	`100% <100%> (+2.27%)`	⬆️
packages/react-hot-loader/src/reconciler/index.js	`100% <100%> (ø)`
packages/react-hot-loader/src/AppContainer.dev.js	`84.21% <100%> (+0.87%)`	⬆️
...-hot-loader/src/reconciler/hotReplacementRender.js	`87.72% <92%> (+0.54%)`	⬆️
...es/react-hot-loader/src/reconciler/proxyAdapter.js	`94.44% <93.93%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2b7c3c...d3e213b. Read the comment docs.

joshjg · 2018-02-05T20:37:44Z

@theKashey

async components will reconcile them self. No need to mark components as hot - they are all hot

Running beta.21, this doesn't seem to be the case. Unless I wrap the async component with hot(module) any change causes it to reload completely.

theKashey · 2018-02-05T20:48:53Z

@joshjg - so two questions:

how you detect and actually load async component
please add this code in any place

 import {setConfig} from 'react-hot-loader';
 setConfig({debug: true});

And the check the console output. Something should be inside.

When you hot async component, you setup HRM to component-will-reload-component. It will perform hot-replacement without loader help. So the problem might be with "loader".

joshjg · 2018-02-05T22:24:02Z

Basically the following:

componentDidMount() {
    import(
        /* webpackChunkName: "[request]" */
        `modules/${someDynamicValue}`
    ).then(({ default: Component }) => this.setState({ Component }));
}

And webpack handles the rest.

I don't see any additional output when using that.

theKashey · 2018-02-05T23:01:39Z

During hot replacement RHL will not trigger componentDidMount. If it got executed - so something "upper" remounts.
Please double check, the problem is before async component.
Do you have any HMR on top level?

joshjg · 2018-02-06T17:14:17Z

Correct, I have the root component on the top level wrapped with hot(module). Changing something in the async child causes this root to reload (unless I wrap the async child with hot(module) as well).

theKashey · 2018-02-06T20:55:38Z

That's mean that React-Hot-Loader is not working for something between top-level component and loader.
Can you double check, that changing top-level component does not cause the problem, and next moving deeper try to find the "unmounting point".
I am quite interested in it.

If possible - just share some code.

joshjg · 2018-02-08T19:50:38Z

The loader component is the one that unmounts. None of its ancestors unmount.

theKashey · 2018-02-08T20:56:51Z

So, @joshjg, could you share some code. There must a reason(or a bug) to unmount.

joshjg · 2018-02-08T21:26:39Z

class ModuleLoader extends React.Component {
    static propTypes = {
        moduleId: PropTypes.string.isRequired,
        moduleName: PropTypes.string.isRequired
    };

    state = { Component: null };

    componentDidMount() {
        this.mounted = true;

        import(
            /* webpackChunkName: "[request]" */
            `modules/${this.props.moduleName}/src`
        ).then(({ default: Component }) => {
            if (this.mounted) {
                this.setState(() => ({ Component }));
            }
        });
    }

    componentWillUnmount() {
        this.mounted = false;
    }

    render() {
        const { Component } = this.state;

        if (Component) {
            return <Component {...this.props}/>;
        }

        return <Placeholder/>;
    }
}

theKashey · 2018-02-08T21:48:15Z

Normal class 🤷‍♂️
Meanwhile I've found a bug which may lead to unmount, and it actually is #843 , but it is not related to sub-render as everyone thought before, and exists from the first beta.

If you will render one component instead of another - RHL will use the old one.

joshjg · 2018-02-12T17:06:04Z

FYI - beta 22 did not fix this issue

theKashey · 2018-02-12T22:18:13Z

@joshjg, in this case, I'll ask for some example to reproduce.

joshjg · 2018-02-13T02:07:10Z

@theKashey Couple things

I messed up my babel config - this was causing the unmounts. After fixing this, the loader no longer unmounts, but instead the async child never updates.
I tried using setConfig({ logLevel: 'debug' }) (instead of debug: true) and found the following error:

React-hot-loader: reconcilation failed due to error TypeError: Cannot read property 'children' of undefined
    at eval (react-hot-loader.development.js:978)

Which is this line: https://github.com/gaearon/react-hot-loader/blob/next/src/reconciler/hotReplacementRender.js#L125

theKashey · 2018-02-13T02:19:46Z

I've got the idea, easy to fix, but lets double check.
Some render returned an array as a children (it could be nested), but in render-before thats child did not exist.

 { someFlag && [<div>....]

Do you have such code?

joshjg · 2018-02-13T02:39:20Z

Yes, I use that pattern all over.

theKashey · 2018-02-13T02:46:03Z

Ok, let me write red tests first.

theKashey added 3 commits January 27, 2018 22:58

add example with async components and portals

2ffa4d6

add hotReplacementRender as a part of each render

bd92941

more complex case

54d71e2

theKashey requested a review from gregberge January 27, 2018 12:44

theKashey mentioned this pull request Jan 29, 2018

Compat mode. #833

Closed

gregberge requested changes Jan 29, 2018

View reviewed changes

theKashey added 2 commits January 30, 2018 17:47

Merge remote-tracking branch 'original/next' into sub-render

3d5b6e7

clean up example + add sub-render test

d3e213b

gregberge merged commit 4e8e617 into next Feb 4, 2018

gregberge deleted the sub-render branch February 4, 2018 21:48

theKashey mentioned this pull request Feb 13, 2018

bug in beta.21 #843

Closed

theKashey mentioned this pull request Apr 2, 2018

Support React 16.3 #918

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sub render #830

Sub render #830

theKashey commented Jan 27, 2018

gregberge left a comment

gregberge Jan 29, 2018

gregberge Jan 29, 2018

gregberge Jan 29, 2018

theKashey Jan 30, 2018

gregberge Jan 29, 2018

theKashey Jan 30, 2018

gregberge Jan 29, 2018

gregberge Jan 29, 2018

gregberge Jan 29, 2018

theKashey Jan 30, 2018

codecov-io commented Jan 30, 2018 •

edited

Loading

joshjg commented Feb 5, 2018

theKashey commented Feb 5, 2018 •

edited

Loading

joshjg commented Feb 5, 2018 •

edited

Loading

theKashey commented Feb 5, 2018 •

edited

Loading

joshjg commented Feb 6, 2018

theKashey commented Feb 6, 2018

joshjg commented Feb 8, 2018

theKashey commented Feb 8, 2018

joshjg commented Feb 8, 2018

theKashey commented Feb 8, 2018

joshjg commented Feb 12, 2018

theKashey commented Feb 12, 2018

joshjg commented Feb 13, 2018

theKashey commented Feb 13, 2018

joshjg commented Feb 13, 2018

theKashey commented Feb 13, 2018

Sub render #830

Sub render #830

Conversation

theKashey commented Jan 27, 2018

gregberge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 30, 2018 • edited Loading

Codecov Report

joshjg commented Feb 5, 2018

theKashey commented Feb 5, 2018 • edited Loading

joshjg commented Feb 5, 2018 • edited Loading

theKashey commented Feb 5, 2018 • edited Loading

joshjg commented Feb 6, 2018

theKashey commented Feb 6, 2018

joshjg commented Feb 8, 2018

theKashey commented Feb 8, 2018

joshjg commented Feb 8, 2018

theKashey commented Feb 8, 2018

joshjg commented Feb 12, 2018

theKashey commented Feb 12, 2018

joshjg commented Feb 13, 2018

theKashey commented Feb 13, 2018

joshjg commented Feb 13, 2018

theKashey commented Feb 13, 2018

codecov-io commented Jan 30, 2018 •

edited

Loading

theKashey commented Feb 5, 2018 •

edited

Loading

joshjg commented Feb 5, 2018 •

edited

Loading

theKashey commented Feb 5, 2018 •

edited

Loading