From f8532df767a842c31204bc18711acd244a40125e Mon Sep 17 00:00:00 2001 From: Anton Date: Mon, 2 Apr 2018 02:50:10 +1000 Subject: [PATCH] fix: double proxy registration (#915) Could break all the things in case of code splitting - something "old" could be "registered" by a new name (it is just a variable name), obtain a new proxy and unmount old instances. Was fixed in v3. We broke it during migration, and had no tests around it. Fix #912 --- src/proxy/createClassProxy.js | 14 ++++++-- src/reconciler/proxies.js | 2 ++ test/AppContainer.dev.test.js | 49 ++++++++++++++++++++++++++++ test/proxy/consistency.test.js | 6 ++++ test/proxy/instance-property.test.js | 8 ++--- test/proxy/static-method.test.js | 11 ++++--- test/proxy/static-property.test.js | 11 ++++--- 7 files changed, 86 insertions(+), 15 deletions(-) diff --git a/src/proxy/createClassProxy.js b/src/proxy/createClassProxy.js index 5d170baae..b4c00c6ed 100644 --- a/src/proxy/createClassProxy.js +++ b/src/proxy/createClassProxy.js @@ -19,7 +19,11 @@ import { inject, checkLifeCycleMethods, mergeComponents } from './inject' const has = Object.prototype.hasOwnProperty -const proxies = new WeakMap() +let proxies = new WeakMap() + +export const resetClassProxies = () => { + proxies = new WeakMap() +} const blackListedClassMembers = [ 'constructor', @@ -174,6 +178,7 @@ function createClassProxy(InitialComponent, proxyKey, options) { let ProxyFacade let ProxyComponent = null + let proxy if (!isFunctionalComponent) { ProxyComponent = proxyClassCreator(InitialComponent, postConstructionAction) @@ -255,10 +260,11 @@ function createClassProxy(InitialComponent, proxyKey, options) { // Prevent proxy cycles const existingProxy = proxies.get(NextComponent) if (existingProxy) { - update(existingProxy[UNWRAP_PROXY]()) return } + proxies.set(NextComponent, proxy) + isFunctionalComponent = !isReactClass(NextComponent) proxyGeneration++ @@ -309,7 +315,9 @@ function createClassProxy(InitialComponent, proxyKey, options) { update(InitialComponent) - const proxy = { get, update } + proxy = { get, update } + + proxies.set(InitialComponent, proxy) proxies.set(ProxyFacade, proxy) safeDefineProperty(proxy, UNWRAP_PROXY, { diff --git a/src/reconciler/proxies.js b/src/reconciler/proxies.js index 4f20dbdab..524c94cbf 100644 --- a/src/reconciler/proxies.js +++ b/src/reconciler/proxies.js @@ -1,4 +1,5 @@ import createProxy from '../proxy' +import { resetClassProxies } from '../proxy/createClassProxy' let proxiesByID let idsByType @@ -35,6 +36,7 @@ export const createProxyForType = type => export const resetProxies = () => { proxiesByID = {} idsByType = new WeakMap() + resetClassProxies() } resetProxies() diff --git a/test/AppContainer.dev.test.js b/test/AppContainer.dev.test.js index 90b62228a..f0f81464f 100644 --- a/test/AppContainer.dev.test.js +++ b/test/AppContainer.dev.test.js @@ -1202,6 +1202,55 @@ describe(`AppContainer (dev)`, () => { expect(wrapper.text()).toBe('TESTnew childnew child') }) + it('renders children with chunked re-register', () => { + const spy = jest.fn() + + class App extends Component { + componentWillUnmount() { + spy() + } + + render() { + return
I AM CHILD
+ } + } + + RHL.reset() + RHL.register(App, 'App1', 'test.js') + RHL.register(App, 'App2', 'test.js') + + const wrapper = mount( + + + , + ) + expect(spy).not.toHaveBeenCalled() + wrapper.setProps({ children: }) + expect(spy).not.toHaveBeenCalled() + RHL.register(App, 'App3', 'test.js') + wrapper.setProps({ children: }) + expect(spy).not.toHaveBeenCalled() + + { + class App extends Component { + componentWillUnmount() { + spy() + } + + render() { + return
I AM NEW CHILD
+ } + } + RHL.register(App, 'App3', 'test.js') + wrapper.setProps({ children: }) + expect(spy).not.toHaveBeenCalled() + + RHL.register(App, 'App4', 'test.js') + wrapper.setProps({ children: }) + expect(spy).not.toHaveBeenCalled() + } + }) + describe('with HOC-wrapped root', () => { it('renders children', () => { const spy = jest.fn() diff --git a/test/proxy/consistency.test.js b/test/proxy/consistency.test.js index 90334cb48..380b4bc50 100644 --- a/test/proxy/consistency.test.js +++ b/test/proxy/consistency.test.js @@ -149,6 +149,12 @@ describe('consistency', () => { expect(proxy.get()).toBe(Proxy) }) + it('prevents double proxy creation', () => { + const proxy1 = createProxy(Bar) + const proxy2 = createProxy(Bar) + expect(proxy1.get()).toBe(proxy2.get()) + }) + it('prevents mutually recursive proxy cycle', () => { const barProxy = createProxy(Bar) const BarProxy = barProxy.get() diff --git a/test/proxy/instance-property.test.js b/test/proxy/instance-property.test.js index bfc7bff01..8356aa68e 100644 --- a/test/proxy/instance-property.test.js +++ b/test/proxy/instance-property.test.js @@ -3,7 +3,7 @@ import React from 'react' import { ensureNoWarnings, createMounter } from './helper' import createProxy from '../../src/proxy' -const fixtures = { +const fixtures = () => ({ modern: { InstanceProperty: class InstanceProperty extends React.Component { answer = 42 @@ -79,7 +79,7 @@ const fixtures = { } }, }, -} +}) describe('instance property', () => { ensureNoWarnings() @@ -91,7 +91,7 @@ describe('instance property', () => { InstanceProperty, InstancePropertyUpdate, InstancePropertyRemoval, - } = fixtures[type] + } = fixtures()[type] it('is available on proxy class instance', () => { const proxy = createProxy(InstanceProperty) @@ -158,7 +158,7 @@ describe('instance property', () => { // }) it('show use the underlayer top value', () => { - const proxy = createProxy(fixtures.modern.InstancePropertyFromContext) + const proxy = createProxy(fixtures().modern.InstancePropertyFromContext) const Proxy = proxy.get() const wrapper = mount() expect(wrapper.text()).toBe('42') diff --git a/test/proxy/static-method.test.js b/test/proxy/static-method.test.js index d12939927..2be2c63fc 100644 --- a/test/proxy/static-method.test.js +++ b/test/proxy/static-method.test.js @@ -2,8 +2,9 @@ import React from 'react' import { ensureNoWarnings, createMounter } from './helper' import createProxy from '../../src/proxy' +import reactHotLoader from '../../src/reactHotLoader' -const fixtures = { +const fixtures = () => ({ modern: { StaticMethod: class StaticMethod extends React.Component { static getAnswer() { @@ -31,19 +32,21 @@ const fixtures = { } }, }, -} +}) describe('static method', () => { ensureNoWarnings() const { mount } = createMounter() - Object.keys(fixtures).forEach(type => { + Object.keys(fixtures()).forEach(type => { describe(type, () => { const { StaticMethod, StaticMethodUpdate, StaticMethodRemoval, - } = fixtures[type] + } = fixtures()[type] + + beforeEach(() => reactHotLoader.reset()) it('is available on proxy class instance', () => { const proxy = createProxy(StaticMethod) diff --git a/test/proxy/static-property.test.js b/test/proxy/static-property.test.js index b262c4639..01a141131 100644 --- a/test/proxy/static-property.test.js +++ b/test/proxy/static-property.test.js @@ -4,8 +4,9 @@ import React from 'react' import PropTypes from 'prop-types' import { ensureNoWarnings, createMounter } from './helper' import createProxy from '../../src/proxy' +import reactHotLoader from '../../src/reactHotLoader' -const fixtures = { +const fixtures = () => ({ modern: { StaticProperty: class StaticProperty extends React.Component { static answer = 42 @@ -65,13 +66,13 @@ const fixtures = { } }, }, -} +}) describe('static property', () => { ensureNoWarnings() const { mount } = createMounter() - Object.keys(fixtures).forEach(type => { + Object.keys(fixtures()).forEach(type => { describe(type, () => { const { StaticProperty, @@ -79,7 +80,9 @@ describe('static property', () => { StaticPropertyRemoval, WithPropTypes, WithPropTypesUpdate, - } = fixtures[type] + } = fixtures()[type] + + beforeEach(() => reactHotLoader.reset()) it('is available on proxy class instance', () => { const proxy = createProxy(StaticProperty)