From 963677f1fad86df89777fe76c32fb1fffc1a6cf3 Mon Sep 17 00:00:00 2001 From: Anton Date: Sun, 11 Feb 2018 05:19:39 +1100 Subject: [PATCH] fix: fix reconciler warnings (#852) * Incriment generation only on replacement event(related to #843) * hotRender: drill into the new children branch(fix #843) * tests for the props change case + ci * react-hot-renderer - optional child case --- examples/styled-components/src/App.js | 5 ++-- src/proxy/utils.js | 5 ++-- src/reactHotLoader.js | 11 ++++++-- src/reconciler/hotReplacementRender.js | 28 +++++++++++++++---- src/reconciler/proxies.js | 3 ++- test/reactHotLoader.test.js | 5 ++++ test/reconciler.test.js | 37 ++++++++++++++++++++++++++ test/reconciler/proxyAdapter.test.js | 30 +++++++++++++++++++++ 8 files changed, 112 insertions(+), 12 deletions(-) create mode 100644 test/reconciler/proxyAdapter.test.js diff --git a/examples/styled-components/src/App.js b/examples/styled-components/src/App.js index 47934ceda..b215693e9 100644 --- a/examples/styled-components/src/App.js +++ b/examples/styled-components/src/App.js @@ -15,7 +15,7 @@ const SmallText = emoStyled('div')` const indirect = { element: () => ( - hidden2 + hidden ), } @@ -24,12 +24,13 @@ const aNumber = 100500 const App = () => (

- 1.Hello, world!! {aNumber} + 1.Hello, world! {aNumber}
2.Hello, world.
+ {aNumber % 2 && }

) diff --git a/src/proxy/utils.js b/src/proxy/utils.js index 58b0d9b8d..133a85a52 100644 --- a/src/proxy/utils.js +++ b/src/proxy/utils.js @@ -44,10 +44,11 @@ export function isNativeFunction(fn) { } export const identity = a => a +const indirectEval = str => [window.eval][0](str) export const doesSupportClasses = (function() { try { - eval('class Test {}') + indirectEval('class Test {}') return true } catch (e) { return false @@ -56,7 +57,7 @@ export const doesSupportClasses = (function() { const ES6ProxyComponentFactory = doesSupportClasses && - eval(` + indirectEval(` (function(InitialParent, postConstructionAction) { return class ProxyComponent extends InitialParent { constructor(props, context) { diff --git a/src/reactHotLoader.js b/src/reactHotLoader.js index e6bd3ca30..7230988a5 100644 --- a/src/reactHotLoader.js +++ b/src/reactHotLoader.js @@ -5,6 +5,7 @@ import { updateProxyById, resetProxies, getProxyByType, + getProxyById, createProxyForType, } from './reconciler/proxies' @@ -27,8 +28,14 @@ const reactHotLoader = { typeof fileName === 'string' && fileName ) { - incrementGeneration() - updateProxyById(`${fileName}#${uniqueLocalName}`, type) + const id = `${fileName}#${uniqueLocalName}` + + if (getProxyById(id)) { + // component got replaced. Need to reconsile + incrementGeneration() + } + + updateProxyById(id, type) } }, diff --git a/src/reconciler/hotReplacementRender.js b/src/reconciler/hotReplacementRender.js index b9335cddf..939c71fa2 100644 --- a/src/reconciler/hotReplacementRender.js +++ b/src/reconciler/hotReplacementRender.js @@ -125,8 +125,18 @@ const mapChildren = (children, instances) => ({ ...mapChildren(child, instances[index].children), } } + const instanceLine = instances[index] || {} + const oldChildren = asArray(instanceLine.children || []) + const newChildren = asArray((child.props && child.props.children) || []) + const nextChildren = + oldChildren.length && mapChildren(newChildren, oldChildren) return { - ...instances[index], + ...instanceLine, + // actually child merge is needed only for TAGs, and usually don't work for Components. + // the children from an instance or rendered children + // while children from a props is just a props. + // they could not exists. they could differ. + ...(nextChildren ? { children: nextChildren } : {}), type: child.type, } }), @@ -146,7 +156,17 @@ const mergeInject = (a, b, instance) => { if (a.length === b.length) { return mapChildren(a, b) } - const flatA = unflatten(a) + + // in some cases (no confidence here) B could contain A except null children + // in some cases - could not. + // this depends on React version and the way you build component. + + const nonNullA = filterNullArray(a) + if (nonNullA.length === b.length) { + return mapChildren(nonNullA, b) + } + + const flatA = unflatten(nonNullA) const flatB = unflatten(b) if (flatA.length === flatB.length) { return mapChildren(flatA, flatB) @@ -239,9 +259,7 @@ const hotReplacementRender = (instance, stack) => { next( // move types from render to the instances of hydrated tree mergeInject( - filterNullArray( - asArray(child.props ? child.props.children : child.children), - ), + asArray(child.props ? child.props.children : child.children), stackChild.instance.children, stackChild.instance, ), diff --git a/src/reconciler/proxies.js b/src/reconciler/proxies.js index fbc2a1b3f..4f20dbdab 100644 --- a/src/reconciler/proxies.js +++ b/src/reconciler/proxies.js @@ -10,7 +10,8 @@ const generateTypeId = () => `auto-${elementCount++}` export const getIdByType = type => idsByType.get(type) -export const getProxyByType = type => proxiesByID[getIdByType(type)] +export const getProxyById = id => proxiesByID[id] +export const getProxyByType = type => getProxyById(getIdByType(type)) export const setStandInOptions = options => { renderOptions = options diff --git a/test/reactHotLoader.test.js b/test/reactHotLoader.test.js index 0d519d3fe..2dbcaa71d 100644 --- a/test/reactHotLoader.test.js +++ b/test/reactHotLoader.test.js @@ -108,6 +108,11 @@ describe('reactHotLoader', () => { it('should increment update counter', () => { const oldGeneration = getGeneration() reactHotLoader.register(Div, 'Div', 'reactHotLoader.test.js') + // new thing, no change + expect(getGeneration()).toBe(oldGeneration + 0) + + reactHotLoader.register(Div, 'Div', 'reactHotLoader.test.js') + // replacement! expect(getGeneration()).toBe(oldGeneration + 1) }) diff --git a/test/reconciler.test.js b/test/reconciler.test.js index b1448496d..cc7521031 100644 --- a/test/reconciler.test.js +++ b/test/reconciler.test.js @@ -12,6 +12,7 @@ const spyComponent = (render, displayName, key) => { const mounted = jest.fn() const unmounted = jest.fn() const willUpdate = jest.fn() + const rendered = jest.fn() class TestingComponent extends Component { componentWillMount() { @@ -34,6 +35,7 @@ const spyComponent = (render, displayName, key) => { /* eslint-enable */ render() { + rendered() return render(this.props) } } @@ -48,6 +50,7 @@ const spyComponent = (render, displayName, key) => { mounted, unmounted, key, + rendered, } } @@ -188,6 +191,40 @@ describe('reconciler', () => { expect(second.mounted).toHaveBeenCalledTimes(0) }) + it('should use new children branch during reconcile', () => { + const First = spyComponent(() => 1, 'test', '1') + const Second = spyComponent(() => 2, 'test', '2') + + const App = ({ second }) => ( +
+
+ + {second && } +
+
+ ) + + const Mounter = ({ second }) => + + const wrapper = mount() + + expect(First.rendered).toHaveBeenCalledTimes(1) + expect(Second.rendered).toHaveBeenCalledTimes(1) + + incrementGeneration() + wrapper.setProps({ update: 'now' }) + expect(First.rendered).toHaveBeenCalledTimes(3) + expect(Second.rendered).toHaveBeenCalledTimes(3) + + incrementGeneration() + wrapper.setProps({ second: false }) + expect(First.rendered).toHaveBeenCalledTimes(5) + expect(Second.rendered).toHaveBeenCalledTimes(3) + + expect(First.unmounted).toHaveBeenCalledTimes(0) + expect(Second.unmounted).toHaveBeenCalledTimes(1) + }) + it('should handle function as a child', () => { const first = spyComponent( ({ children }) => {children(0)}, diff --git a/test/reconciler/proxyAdapter.test.js b/test/reconciler/proxyAdapter.test.js new file mode 100644 index 000000000..5f9293a1d --- /dev/null +++ b/test/reconciler/proxyAdapter.test.js @@ -0,0 +1,30 @@ +/* eslint-env browser */ +import { proxyWrapper } from '../../src/reconciler/proxyAdapter' +import * as proxies from '../../src/reconciler/proxies' + +jest.mock('../../src/reconciler/proxies') + +proxies.getProxyByType.mockReturnValue({ get: () => 'proxy' }) + +describe(`proxyAdapter`, () => { + const fn = () => {} + + it('should handle empty result', () => { + expect(proxyWrapper()).toBe(undefined) + expect(proxyWrapper(null)).toBe(null) + }) + + it('should handle arrays', () => { + expect(proxyWrapper([1, 2, 3])).toEqual([1, 2, 3]) + expect(proxyWrapper([{ type: fn, prop: 42 }])).toEqual([ + { type: 'proxy', prop: 42 }, + ]) + }) + + it('should handle elements', () => { + expect(proxyWrapper({ type: fn, prop: 42 })).toEqual({ + type: 'proxy', + prop: 42, + }) + }) +})