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

react-hot-loader compatibility #1336

Closed
arqex opened this issue May 3, 2018 · 7 comments
Closed

react-hot-loader compatibility #1336

arqex opened this issue May 3, 2018 · 7 comments

Comments

@arqex
Copy link

arqex commented May 3, 2018

Hey guys,

We are trying to make preact compatible with RHL and it would be great to play along with it too:

preactjs/preact#1007

After having a look at Inferno and RHL together, the problems are very similar to the ones seen in preact. I am using inferno-compat and most of the components are working ok, but there are some problems using higher order components.

The way RHL works is hijack createElement function, transparently to inferno, and instead of returning an instance of the element, it creates a proxy of it (a slightly different version) that is updated whenever a new hot module is injected.

The proxy is almost the same than the real component, but not exactly, so these comparisons in inferno fail:

if (lastVNode.type !== nextTag) {

if (lastVNode.type !== nextType || lastKey !== nextKey) {

lastVNode.type === nextVNode.type is not true anymore, the result is that the component get unmounted and replacing by a new one, losing its internal state.

RHL has its ways of knowing when two proxies belong to the same component, so it would be great that inferno could have some way to configure the comparison function. If inferno exported some kind of setComponentComparator method, RHL would be able to hook in there in order to say when a component need to be flushed.

No more changes would be needed for inferno + inferno-compat to work with RHL.

If we wanted RHL work without inferno's compatibility layer, it would need also a way of hijacking the creation of elements to inject the proxies. I don't know if that's already possible though.

@Havunen
Copy link
Member

Havunen commented May 3, 2018

Hi,

I like the idea of supporting RHL. How does React handle this situation? I was just reading React source code and they do also have child.type === element.type check in place for Components.

https://github.com/facebook/react/blob/8dc8f88d5ae9fb96934ba43e3842b5dcf4074afd/packages/react-reconciler/src/ReactChildFiber.js#L1101-L1120

@arqex
Copy link
Author

arqex commented May 3, 2018

Good catch! I think that RHL is handling the whole re-rendering process for react. @theKashey can probably tell us more.

@theKashey
Copy link

theKashey commented May 3, 2018

The main problem - if you will follow "react" way - you will hit our main problem - gaearon/react-hot-loader#304

const element = <ImportedComponent />
console.log(element.type === ImportedComponent) // false

This is not a problem for react/preact/inferno, as long they are comparing result of createElement, but for a developer and third-party libraries.

After creating the proxy RHL just maintain it, patching with a new code in right moments. Finding "the right moment" is a pain, and finding "the correct pair" to compare - is a pain.

Custom element comparator washes it away.

And there is only one this lasts to answer the question...

How the ideal React-Hot-Loader should work?

  1. Compare elements by configurable function. RHL compare could work with wrapped or unwrapped elements, as long we could wrap/unwrap them by ourselves. (See PR in Preact)
  2. react/preact/inferno should wrap element with proxy just before rendering. Like rendering 'div'(string) to div(tag) - that transformation already exists. No more hacks into the exported names. Just export one more function to configure.

The problem, why RHL not working for Preact, or Inferno, ie

if (lastVNode.type !== nextTag) { 

is about createElement. When you call that function RHL is trying to find a proxy for a Type. It works for "top level" types ("registered" via babel plugin), but not for HOCs). All HOCs will get auto-NUMBER proxy - a new each time, thus failing comparison.

As long createElement goes first, and compare goes second - it is a problem. But if compare will be first, createElement will be normal, and only "real render" will render a bit different type - everything will be better. I hope.

PS: Just a setComponentComparator function will make Inferno 100% compatible with RHL. I am just looking for even better experience, as long h(createElement) is not always hookable.

@Havunen
Copy link
Member

Havunen commented Sep 11, 2018

I think we could add this option to v6. Maybe its enough If custom comparator logic is only used in development build? The thing is this logic needs to go into patch routine which is called for every vNode update.

import { options } from 'inferno';

options.componentCompator = customFunction;

Then return value would be isDifferent? True -> replaces component with new one and false would do update.

@theKashey
Copy link

I would say - that's the best way. RHL is dev time tool, and shall not slow down anything in production, and element comparison is a quite hot path.

@Havunen
Copy link
Member

Havunen commented Sep 18, 2018

I added the option to dev branch. It will be available in v6

@Havunen
Copy link
Member

Havunen commented Oct 13, 2018

I'm closing this ticket as Inferno v6 is now released! 🎉

https://github.com/infernojs/inferno/releases/tag/v6.0.0

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

No branches or pull requests

3 participants