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

Preact flushing entire components using react-hot-loader #1007

Closed
arqex opened this issue Feb 15, 2018 · 30 comments
Closed

Preact flushing entire components using react-hot-loader #1007

arqex opened this issue Feb 15, 2018 · 30 comments

Comments

@arqex
Copy link
Contributor

arqex commented Feb 15, 2018

Hi guys,

I had a tough day trying to make preact work with react-hot-loader v3 but at least I have discovered why.

The problem I had was that preact was flushing an entire form component when typing on an input, it was supposed to just re-render the input. Unmounting and mounting the component make the input lose the focus and the form is not usable anymore.

After much investigating the problem was that react-hot-loader uses proxies in order to create instances of the components, see:
https://github.com/gaearon/react-hot-loader/blob/master/src/patch.dev.js#L106

Using RHL, when a component is created using inst = new Ctor(props, context) by preact we are not getting an instance of Ctor, but an instance of a copy of Ctor created by the proxy.

That makes that inst.constructor !== Ctor so the next time that the component get rendered, preact tries to build it from the VNode and finds that dom._componentConstructor !== vnode.nodeName and it decides to unmount it.

I understand this is not preact's fault, but it's so easy to fix just modifying one line of code of preact, overriding inst.constructor = Ctor for all components:

In component-recycler.js:

if (Ctor.prototype && Ctor.prototype.render) {
  inst = new Ctor(props, context);
  Component.call(inst, props, context);
} else {
  inst = new Component(props, context);
  inst.render = doRender;
}

// Override the constructor even if the instance was created by `new Ctor()`
// This will make preact work ok with react-hot-loader using proxies to create instances
inst.constructor = Ctor;

If I create a PR would you merge it? It's my missing piece to finally make preact and react-hot-module work together!

Thanks for preact, it's great!

@arqex
Copy link
Contributor Author

arqex commented Feb 16, 2018

A temporary hacky fix I use to make RHL work properly with preact in the meanwhile, remember that if you use componentWillMount in your component you will need to add this piece of code there too:

// We are using preact-compat
import React from 'react';

// Fix react-hot-loader with preact
Component.prototype.componentWillMount = function(){

    // Use react's create element to let RHL proxy the component
    var vnode = React.createElement(this.constructor);
    this.constructor = vnode.nodeName;
}

Edited to override componentWillMount instead componentDidMount because it was still giving problems.

@developit
Copy link
Member

Ahhh, interesting! We could probably justify adding that line as long as it doesn't have a huge performance impact. I'd be happy to look at a PR if you want to submit one.

@marvinhagemeister
Copy link
Member

We had to revert #1016 due to possible performance concerns.

@developit
Copy link
Member

If anyone has a neat workaround for this let us know!

Currently I'm thinking we'll need to add a duplicate of the constructor property to all component instances and switch to using that in Preact's checks (avoiding the deopt).

@theKashey
Copy link

React-Hot-Loader maintainer here.
Look like this issue was created to handle preact + RHL v3, while v4 is a bit different.
Probably there is nothing you have to change, but something we should.

@arqex
Copy link
Contributor Author

arqex commented Apr 27, 2018

Last month I was decided to add real support for react-hot-loader for preact and also inferno, so I created this playground:

https://github.com/passpill-io/react-preact-inferno-hot-boilerplate

I've been busy in the last month and I couldn't even start working on it, but I had enough time to check that the fix I proposed here wasn't working well for RHL4 either, so glad you guys didn't merge it.

@theKashey the problem is that react-hot-loader relies a lot in how react rendering internals work, I could see differences for the fiber and classic rendering algorithms. Preact and inferno and internals are really different from react one, so I would either create forks from rhl than adding support for preact and inferno in it.

@theKashey
Copy link

@arqex - we have "adapters" in form of different "hydrate-stack" for React 15 and 16.
We just have to found a way to traverse Preact tree, and job is done.

@arqex
Copy link
Contributor Author

arqex commented Apr 29, 2018

@theKashey so let's give it a try to create those adapters. I had some time in a train to play with RHL code and I have added different RHL source files for every library to the testing repo, so when run some development server RHL is loaded from here:

https://github.com/passpill-io/react-preact-inferno-hot-boilerplate/tree/master/src/rhl

When using preact, the error is hit at the getReactStack function.

https://github.com/gaearon/react-hot-loader/blob/master/src/internal/getReactStack.js

I think this is the point where different adapters are loaded, rootNode is undefined for preact, so we would need a way of knowing that we are using preact at that point and load its own adapter.

I don't have a clear idea of how RHL work yet. Is it there when RHL traverse the component tree updating the proxies to re-render the new versions?

@theKashey
Copy link

@arqex - everything is correct.
The goal is to "hydrate" tree(getReactStack), and then RHL will run replacementRender comparing freshly rendered elements to hydrated ones.
It is not so easy, as long react "optimizes"(squashes) render result a bit, and we had to add some hacks to make them comparable.
And, as I found, those hacks become bad citizens for Preact.

@theKashey
Copy link

Let me finish spike first. It will not take too long. But proper testing - will, and will require your help.

@arqex
Copy link
Contributor Author

arqex commented Apr 30, 2018

Hey @theKashey so good you had a look at it and open an issue in the RHL repo too.

Getting the stack shouldn't be really difficult, I think the key part is knowing when two components are the same, and that might need changes also in preact. If you see the comparison here:

https://github.com/developit/preact/blob/0a88752e4890d391b5e23809d9432f625a833ae8/src/vdom/component.js#L206

https://github.com/developit/preact/blob/0a88752e4890d391b5e23809d9432f625a833ae8/src/vdom/component.js#L218-L221

Preact decides to unmount any component if dom._componentConstructor===vnode.nodeName. For what I have seen in RHL code, it's not possible to override the constructor, so the left part of the comparison refers to the original Component and the right part to the Proxy, so Preact is always unmounting the stack after hydrating (that's what was happening with RHL 3).

@theKashey
Copy link

They both should be Proxyies. React-Hot-Loader "starts" from overriding createElement to rule element creation.
https://github.com/gaearon/react-hot-loader/blob/master/src/reactHotLoader.js#L52

@theKashey
Copy link

@arqex - and I still could not find the "entry point" to stack.
"Stack" for RHL - the mounted components, I could diff with.
I have this, I could dive into this._component, and access children as it done here, but something is wrong - "my" children are empty.

@theKashey
Copy link

Oh no. There is no VDOM here!

@theKashey
Copy link

theKashey commented Apr 30, 2018

Ok. So here is the plan. I'll describe the problems, and a few ways to solve them.

How RHL v3 works?

It uses babel to "register" all top level variables (including Components), and then replace "real" ones by "fake" on createElement call.

The problems here - HOCs and decorators. They are not a "single" top level variable, usually they are top level+internal. And that internal is invisible for v3.

What did v4 changed

Exactly this moment. It first "hydrates" the tree, and then starts traversing the old and the new tree simultaneously. But with preact we don't have the old tree to traverse.

How to solve?!

I have 2 ideas, both are great, both will work.

Idea 1. An easy one.

  1. Completely disable RHL "hot-renderer"
  2. Make "compare" function configurable in preact (in "debug" mode).
  3. Append this logic from RHL to a new comparator.

Then, on element comparison, it will not dom._componentConstructor===vnode.nodeName, but will isSwappable(dom._componentConstructor, vnode.nodeName) and next proxy got updated, original class will be hacked, and magic happened.

Actually - this is hot react-hot-loader should work from the scratch. Could be easy implemented both in React and Preact. As result it could compare existing component with a potential one, and diside - should it be hot-replaced or not. Still waiting for @gaearon judgement around this "more deeper" integration.

But it will require changes in base libraries.

Idea 2. An enterprise one.

  1. Remove all "getStack" related code. We will not need it anymore.
  2. On "hot-render" render the old tree, then render the new tree, as usual.
  3. As long we render everything by ourself - we could remove 50% of "hacks" from hot renderer and comparison will be super easy.
  4. Result is React/Preact version agnostic, and does not rely on any internals.

The problem here - how render the old tree. It's doable, as long we control createElement and all "entry points" are registered via babel plugin.

So - on some component render, we can detect that hot-replacement-render should be run, and then "roll back" the proxy registrations, thus being able to render the old tree.
Could be easily made if actual "componentSwap" will not occur on registration, but on createElement call, it will remove requirement of "unrolling" as long nothing will be applied yet.

Both ways could reduce technical debt and greatly simplify how RHL works.
But which way to follow?
@neoziro?

@arqex
Copy link
Contributor Author

arqex commented Apr 30, 2018

@theKashey I am not an expert in preact internals world either but seems that preact doesn't store the vdom once the dom has been rendered. On re-rendering the diff is done between the new vdom and the real one. Other guys in the thread will be of much more help in that.

But surprisingly, RHL4 plays much better with preact than the v3. If we just return an empty stack {} from the getReactStack function, letting preact update the dom, hot replacement seems to work. Components are updated preserving their internal state, at least in the test repo.

Same trick doesn't work with inferno.js though.

@marvinhagemeister
Copy link
Member

@arqex you're right, we diff against the real dom, not between two vdom trees like react does.

@theKashey
Copy link

Ok, so let me just make RHL work with Preact in "v3" mode, and then we will try recover a better experience.

@arqex
Copy link
Contributor Author

arqex commented May 1, 2018

@theKashey it's the opposite, it's working really well with v4. Just returning an empty stack from getReactStack. Preact re-renders the dom nicely, preserving the state.

@theKashey
Copy link

So pull request is opened. But before I'll merge it - let's talk a bit.

With the change RHL will work with preact-compact out of the box, will require some configuration for "jsx preact.h", and will not work for "jsx h", as long I could not hook into named export. And "result" will not be "HOC-friendly".

To make it "friendly" - "idea 1" (zero changes for RHL, 3 lines of code for preact) or "idea 2" (50 lines in RHL) have to be implemented.

Overriding of createElement function is crucial, but maybe there is more "preact" way to do it. Look like named imports (ie just "h") are popular, and I could not handle them at all.

@arqex
Copy link
Contributor Author

arqex commented May 1, 2018

How could I add some HOC component to the test repo in order to see what's not working? I don't understand well the changes we should make to RHL or preact.

@theKashey
Copy link

@arqex - I've opened another PR, to spike idea 1.
Let me explain everything there.
gaearon/react-hot-loader#960

@arqex
Copy link
Contributor Author

arqex commented May 3, 2018

@theKashey Named imports are always a pain to hack :)

I have added the method setComponentComparator to my fork, you can try and check if it's enough to make HOComponents work. To install it:

$ npm install --save  https://github.com/arqex/preact.git

I don't know why I didn't get it built when I install the fork. If it doesn't work for you either you can try...

$ cd node_modules/preact
$ npm install
$ npm run build

I tested it with an indirect component like the one you have in the PR, and a comparator function like this:

import { setComponentComparator } from 'preact';
setComponentComparator( (node, vnode ) => {
  return node._componentConstructor.displayName === vnode.nodeName.displayName;  
});

And voilà! it worked with my example, preserving the state of my HOC. Using displayName is too simplistic, and it probably will fail in most scenarios, but it a good start point to try your changes.

@marvinhagemeister here the changes I made to preact: master...arqex:master

As you can see, we only need a way of customize component comparison, as dom._componentConstructor !== vnode.nodeName is false when using react-hot-loader. I am not sure how you guys like to organize the code or tune it up for production bundles (all this is probably not needed at all in production), it may be improved.

@theKashey
Copy link

We are not working in production :)
And I have to double check what means "mount/unmount" for preact.
Look like I could stop doing anything around SFA, but still have to hack around Classes, as long I have patch them, not just render.

@developit
Copy link
Member

Not sure if it's useful, but Preact exports createElement() as an alias of h() since version 7.

Also, we provide a way to intercept VNode creation that might be useful here:

import { options } from 'preact'

let old = options.vnode
options.vnode = vnode => {
  // you can mutate vnodes here rather than overriding createElement.

  // vnode.nodeName = ??

  if (old) old(vnode)
}

@theKashey
Copy link

theKashey commented May 4, 2018

@developit - will it make "proxing" only the internal thing? IE not exposing the fact of type replacement, thus making <ImportedComponent />.type === ImportedComponent?

patched createElement will wrap type with proxy and this comparison will fail.

Is it "the perfect way" I've described on infernojs/inferno#1336 (comment)

@theKashey
Copy link

No, it is not :(
Could you think about hooking vnode at hidden from a user level?

@developit
Copy link
Member

@theKashey any idea if it would be possible to solve this by using some sort of internal component mapping in RHL? Like watching for module.hot.accept, and adding Component constructor aliases as constructors are replaced at module boundaries.

@theKashey
Copy link

No. There are 3 problems I need to point on:

  • Who to proxy? RHL have to add some sugar into Component, stateful or stateless. createElement helps distinguish plain javascript functions and stateless components, returning a corresponding proxy, when asked. That's already done and working.
  • Who to replace? RHL uses babel or webpack plugins to extract top-level module variables, and register them to the module boundaries. When another component got registered instead of the original one - it will be replaced in Application. So it is already working as you proposed. Since forever, ie version 1.
  • Who to replace strikes back! Not all components are top-level variables. HOC, decorators, DI(in styled components) - they all could shadow "real" component, or just create it in the local scope. RHL v4 fixes this by traversing React tree, and "updating" components, not visible to babel plugin. As long Preact doesn't use VDOM - this approach does not work, and I had to draft Add a new option to control element comparison #1120, which actually do the same - updating components while traversing rendered tree.

So - right now this issue is partially mitigated, to the level of React-Hot-Loader 3 capabilities. That's 80% of use cases. Unfortunately - the first component, RHL could not handle, will wipe all the nested tree. Sometimes that 80% does not work at all.

@marvinhagemeister
Copy link
Member

marvinhagemeister commented Aug 10, 2020

FYI: We now have an official HMR solution for Preact called prefresh. We recommend everyone switching over to use that 🎉

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

4 participants