Merge pull request #1282 from gaearon/fix-memo

Fix React.memo

src/fresh/babel.js

@@ -202,7 +202,7 @@ export default function(babel) {
       key: fnHookCalls.map(call => call.name + '{' + call.key + '}').join('\n'),
       customHooks: fnHookCalls
         .filter(call => !isBuiltinHook(call.name))
-        .map(call => t.clone(call.callee)),
+        .map(call => t.cloneDeep(call.callee)),
     };
   }
 

src/reconciler/componentComparator.js

@@ -1,11 +1,4 @@
-import {
-  getIdByType,
-  getProxyByType,
-  getSignature,
-  isColdType,
-  isRegisteredComponent,
-  updateProxyById,
-} from './proxies';
+import { getIdByType, getProxyByType, getSignature, isColdType, updateProxyById } from './proxies';
 import { hotComparisonOpen } from '../global/generation';
 import { isForwardType, isMemoType, isReactClass, isReloadableComponent } from '../internal/reactUtils';
 import { areSwappable } from './utils';
@@ -64,23 +57,46 @@ const areSignaturesCompatible = (a, b) => {
 };
 
 const compareRegistered = (a, b) => {
-  if (isRegisteredComponent(a) || isRegisteredComponent(b)) {
-    if (resolveType(a) !== resolveType(b)) {
+  if (getIdByType(a) === getIdByType(b)) {
+    if (getProxyByType(a) !== getProxyByType(b)) {
       return false;
     }
   }
 
   return areSignaturesCompatible(a, b);
 };
 
+const areDeepSwappable = (oldType, newType) => {
+  const type = { type: oldType };
+
+  if (typeof oldType === 'function') {
+    return areSwappable(oldType, newType);
+  }
+
+  if (isForwardType(type)) {
+    return areDeepSwappable(oldType.render, newType.render);
+  }
+
+  if (isMemoType(type)) {
+    return areDeepSwappable(oldType.type, newType.type);
+  }
+
+  // that's not safe
+  // if (isLazyType(type)) {
+  //   return areDeepSwappable(oldType._ctor, newType._ctor)
+  // }
+
+  return false;
+};
+
 const compareComponents = (oldType, newType, setNewType, baseType) => {
   let defaultResult = oldType === newType;
 
   if ((oldType && !newType) || (!oldType && newType)) {
     return false;
   }
 
-  if (isRegisteredComponent(oldType) || isRegisteredComponent(newType)) {
+  if (getIdByType(oldType)) {
     if (!compareRegistered(oldType, newType)) {
       return false;
     }
@@ -91,7 +107,7 @@ const compareComponents = (oldType, newType, setNewType, baseType) => {
     if (!compareRegistered(oldType.render, newType.render)) {
       return false;
     }
-    if (oldType.render === newType.render || areSwappable(oldType.render, newType.render)) {
+    if (oldType.render === newType.render || areDeepSwappable(oldType, newType)) {
       setNewType(newType);
       return true;
     }
@@ -102,7 +118,7 @@ const compareComponents = (oldType, newType, setNewType, baseType) => {
     if (!compareRegistered(oldType.type, newType.type)) {
       return false;
     }
-    if (oldType.type === newType.type || areSwappable(oldType.type, newType.type)) {
+    if (oldType.type === newType.type || areDeepSwappable(oldType, newType)) {
       if (baseType) {
         // memo form different fibers, why?
         if (baseType.$$typeof === newType.$$typeof) {
@@ -125,8 +141,9 @@ const compareComponents = (oldType, newType, setNewType, baseType) => {
   }
 
   if (
-    defaultResult ||
-    (newType !== oldType && areSignaturesCompatible(newType, oldType) && areSwappable(newType, oldType))
+    typeof newType === 'function' &&
+    (defaultResult ||
+      (newType !== oldType && areSignaturesCompatible(newType, oldType) && areSwappable(newType, oldType)))
   ) {
     const unwrapFactory = newType[UNWRAP_PROXY];
     const oldProxy = unwrapFactory && getProxyByType(unwrapFactory());
@@ -150,8 +167,11 @@ export const hotComponentCompare = (oldType, preNewType, setNewType, baseType) =
   const newType = configuration.intergratedResolver ? resolveType(preNewType) : preNewType;
   let result = oldType === newType;
 
+  if (!hotActive) {
+    return result;
+  }
+
   if (
-    result ||
     !isReloadableComponent(oldType) ||
     !isReloadableComponent(newType) ||
     isColdType(oldType) ||

src/reconciler/proxies.js

@@ -37,10 +37,10 @@ export const updateFunctionProxyById = (id, type, updater) => {
   idsByType.set(type, id);
   const proxy = proxiesByID[id];
   if (!proxy) {
-    idsByType.set(type, id);
     proxiesByID[id] = type;
   }
   updater(proxiesByID[id], type);
+  // proxiesByID[id] = type; // keep the first ref
 
   return proxiesByID[id];
 };

test/hot/react-dom.integration.spec.js

@@ -1,12 +1,13 @@
 /* eslint-env browser */
 /* eslint-disable no-lone-blocks,  global-require, prefer-template, import/no-unresolved */
 import 'babel-polyfill';
-import React from 'react';
+import React, { Suspense } from 'react';
 import ReactDom from 'react-dom';
 // import TestRenderer from 'react-test-renderer';
 import ReactHotLoader, { setConfig } from '../../src/index.dev';
 import { configureGeneration, incrementHotGeneration } from '../../src/global/generation';
 import configuration from '../../src/configuration';
+import { AppContainer } from '../../index';
 
 jest.mock('react-dom', () => require('./react-dom'));
 
@@ -21,7 +22,7 @@ describe(`🔥-dom`, () => {
 
   const tick = () => new Promise(resolve => setTimeout(resolve, 10));
 
-  if (React.useContext) {
+  if (React.useContext && String(() => 42).indexOf('=>') > 0) {
     it('shall integrate with React', () => {
       expect(ReactHotLoader.IS_REACT_MERGE_ENABLED).toBe(true);
       expect(configuration.intergratedResolver).toBe(true);
@@ -64,7 +65,7 @@ describe(`🔥-dom`, () => {
       expect(unmount).not.toHaveBeenCalled();
     });
 
-    it.only('should fail component comparator', async () => {
+    it('should fail component comparator', async () => {
       const mount = jest.fn();
       const unmount = jest.fn();
       const Fun1 = () => {
@@ -197,13 +198,16 @@ describe(`🔥-dom`, () => {
           return 'test1' + this.state.x;
         }
       }
+
       Comp1.displayName = 'Comp1';
 
       const el = document.createElement('div');
       ReactDom.render(<Comp1 />, el);
 
       expect(el.innerHTML).toEqual('test11');
-      expect(<Comp1 />.type).toBe(Comp1);
+      if (configuration.intergratedResolver) {
+        expect(<Comp1 />.type).toBe(Comp1);
+      }
 
       incrementHotGeneration();
       {
@@ -216,6 +220,7 @@ describe(`🔥-dom`, () => {
             return 'test2' + this.state.x;
           }
         }
+
         Comp1.displayName = 'Comp1';
 
         ReactDom.render(<Comp1 />, el);
@@ -242,7 +247,9 @@ describe(`🔥-dom`, () => {
       ReactDom.render(<Comp1 />, el);
 
       expect(el.innerHTML).toEqual('test11');
-      expect(<Comp1 />.type).toBe(Comp1);
+      if (configuration.intergratedResolver) {
+        expect(<Comp1 />.type).toBe(Comp1);
+      }
 
       incrementHotGeneration();
       {
@@ -263,6 +270,61 @@ describe(`🔥-dom`, () => {
 
       expect(el.innerHTML).toEqual('test21');
     });
+
+    it('support lazy memo forward', () => {
+      const spy = jest.fn();
+      const sandbox = x => {
+        const Comp = () => {
+          React.useEffect(() => () => spy(), []);
+          return <div>lazy {x}</div>;
+        };
+        ReactHotLoader.register(Comp, 'S1Comp', 'test');
+        // use sync importer
+        const importer = {
+          then(x) {
+            const result = x({ default: Comp });
+            return {
+              then(cb) {
+                cb(result);
+              },
+            };
+          },
+        };
+
+        const Lazy = React.lazy(() => importer);
+        ReactHotLoader.register(Lazy, 'S1Lazy', 'test');
+        const Forward = React.forwardRef(() => (
+          <Suspense fallback="loading">
+            <Lazy />
+          </Suspense>
+        ));
+        const Memo = Forward; // React.memo(Forward);
+        ReactHotLoader.register(Memo, 'S1Memo', 'test');
+        return () => (
+          <AppContainer>
+            <Memo />
+          </AppContainer>
+        );
+      };
+
+      const S1 = sandbox(1);
+      ReactHotLoader.register(S1, 'S1', 'test');
+
+      const el = document.createElement('div');
+      ReactDom.render(<S1 />, el);
+      expect(el.innerHTML).toEqual('<div>lazy 1</div>');
+      expect(spy).not.toHaveBeenCalled();
+
+      incrementHotGeneration();
+
+      const S2 = sandbox(2);
+      ReactHotLoader.register(S2, 'S1', 'test');
+
+      ReactDom.render(<S2 />, el);
+
+      expect(el.innerHTML).toEqual('<div>lazy 2</div>');
+      expect(spy).not.toHaveBeenCalled();
+    });
   } else {
     it('target platform does not support useContext', () => {
       expect(true).toBe(true);