From d796af85519049c4b56b3eb2261049b5a8e8d5da Mon Sep 17 00:00:00 2001 From: Anton Korzunov Date: Wed, 3 Jul 2019 20:59:54 +1000 Subject: [PATCH 1/2] fix: use deep clone for fresh signature, fixes #1280 --- src/fresh/babel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fresh/babel.js b/src/fresh/babel.js index 5333aa382..64836e3c8 100644 --- a/src/fresh/babel.js +++ b/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)), }; } From 5bca98c4884f05a58944b56868633d5299b22423 Mon Sep 17 00:00:00 2001 From: Anton Korzunov Date: Wed, 3 Jul 2019 21:00:13 +1000 Subject: [PATCH 2/2] fix: memo components are not updated --- src/reconciler/componentComparator.js | 52 +++++++++++++------ src/reconciler/proxies.js | 2 +- test/hot/react-dom.integration.spec.js | 72 ++++++++++++++++++++++++-- 3 files changed, 104 insertions(+), 22 deletions(-) diff --git a/src/reconciler/componentComparator.js b/src/reconciler/componentComparator.js index 17e42aefe..91606ce8b 100644 --- a/src/reconciler/componentComparator.js +++ b/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,8 +57,8 @@ 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; } } @@ -73,6 +66,29 @@ const compareRegistered = (a, b) => { 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; @@ -80,7 +96,7 @@ const compareComponents = (oldType, newType, setNewType, baseType) => { 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) || diff --git a/src/reconciler/proxies.js b/src/reconciler/proxies.js index 6411737e8..b3681da73 100644 --- a/src/reconciler/proxies.js +++ b/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]; }; diff --git a/test/hot/react-dom.integration.spec.js b/test/hot/react-dom.integration.spec.js index a20b7d4aa..677d65868 100644 --- a/test/hot/react-dom.integration.spec.js +++ b/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(, el); expect(el.innerHTML).toEqual('test11'); - expect(.type).toBe(Comp1); + if (configuration.intergratedResolver) { + expect(.type).toBe(Comp1); + } incrementHotGeneration(); { @@ -216,6 +220,7 @@ describe(`🔥-dom`, () => { return 'test2' + this.state.x; } } + Comp1.displayName = 'Comp1'; ReactDom.render(, el); @@ -242,7 +247,9 @@ describe(`🔥-dom`, () => { ReactDom.render(, el); expect(el.innerHTML).toEqual('test11'); - expect(.type).toBe(Comp1); + if (configuration.intergratedResolver) { + expect(.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
lazy {x}
; + }; + 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(() => ( + + + + )); + const Memo = Forward; // React.memo(Forward); + ReactHotLoader.register(Memo, 'S1Memo', 'test'); + return () => ( + + + + ); + }; + + const S1 = sandbox(1); + ReactHotLoader.register(S1, 'S1', 'test'); + + const el = document.createElement('div'); + ReactDom.render(, el); + expect(el.innerHTML).toEqual('
lazy 1
'); + expect(spy).not.toHaveBeenCalled(); + + incrementHotGeneration(); + + const S2 = sandbox(2); + ReactHotLoader.register(S2, 'S1', 'test'); + + ReactDom.render(, el); + + expect(el.innerHTML).toEqual('
lazy 2
'); + expect(spy).not.toHaveBeenCalled(); + }); } else { it('target platform does not support useContext', () => { expect(true).toBe(true);