From 6f88c133d4a21e7b1de1dab140c2627774a6b4e0 Mon Sep 17 00:00:00 2001 From: Shlomi Scemes Date: Tue, 4 Feb 2020 10:52:35 +0200 Subject: [PATCH] fixed connect issue with stale props (zombie children) --- package.json | 4 ++-- src/index.js | 24 ++++----------------- src/integrations/preact.js | 5 +++++ src/integrations/react.js | 6 ++++++ src/subscriber.js | 35 ++++++++++++++++++++++++++++++ test/preact/preact.test.js | 44 ++++++++++++++++++++++++++++++++++++++ test/react/react.test.js | 42 ++++++++++++++++++++++++++++++++++++ 7 files changed, 138 insertions(+), 22 deletions(-) create mode 100644 src/subscriber.js diff --git a/package.json b/package.json index 60f688e..b16bbc4 100644 --- a/package.json +++ b/package.json @@ -28,7 +28,7 @@ "bundlesize": [ { "path": "full/preact.js", - "maxSize": "760b" + "maxSize": "850b" }, { "path": "dist/unistore.js", @@ -36,7 +36,7 @@ }, { "path": "preact.js", - "maxSize": "600b" + "maxSize": "700b" } ], "babel": { diff --git a/src/index.js b/src/index.js index 4df2006..cc978cb 100644 --- a/src/index.js +++ b/src/index.js @@ -1,4 +1,5 @@ import { assign } from './util'; +import createSubscriber from './subscriber'; /** * Creates a new store, which is a tiny evented state container. @@ -12,26 +13,12 @@ import { assign } from './util'; * store.setState({ c: 'd' }); // logs { a: 'b', c: 'd' } */ export default function createStore(state) { - let listeners = []; + const { emit, subscribe, unsubscribe } = createSubscriber(); state = state || {}; - function unsubscribe(listener) { - let out = []; - for (let i=0; i { unsubscribe(listener); }; - }, + subscribe, /** * Remove a previously-registered listener function. diff --git a/src/integrations/preact.js b/src/integrations/preact.js index 81c09e3..768e3b4 100644 --- a/src/integrations/preact.js +++ b/src/integrations/preact.js @@ -1,4 +1,5 @@ import { h, Component } from 'preact'; +import createSubscriber from '../subscriber'; import { assign, mapActions, select } from '../util'; /** @@ -22,6 +23,8 @@ export function connect(mapStateToProps, actions) { return Child => { function Wrapper(props, context) { const store = context.store; + const { emit, subscribe, unsubscribe } = createSubscriber(); + const subStore = { ...store, subscribe, unsubscribe }; let state = mapStateToProps(store ? store.getState() : {}, props); const boundActions = actions ? mapActions(actions, store) : { store }; let update = () => { @@ -34,6 +37,7 @@ export function connect(mapStateToProps, actions) { state = mapped; return this.setState({}); } + emit(); }; this.componentWillReceiveProps = p => { props = p; @@ -45,6 +49,7 @@ export function connect(mapStateToProps, actions) { this.componentWillUnmount = () => { store.unsubscribe(update); }; + this.getChildContext = () => ({ store: subStore }); this.render = props => h(Child, assign(assign(assign({}, boundActions), props), state)); } return (Wrapper.prototype = new Component()).constructor = Wrapper; diff --git a/src/integrations/react.js b/src/integrations/react.js index 6b1fd4c..940c1ca 100644 --- a/src/integrations/react.js +++ b/src/integrations/react.js @@ -1,4 +1,5 @@ import { createElement, Children, Component } from 'react'; +import createSubscriber from '../subscriber'; import { assign, mapActions, select } from '../util'; const CONTEXT_TYPES = { @@ -26,6 +27,8 @@ export function connect(mapStateToProps, actions) { function Wrapper(props, context) { Component.call(this, props, context); const store = context.store; + const { emit, subscribe, unsubscribe } = createSubscriber(); + const subStore = { ...store, subscribe, unsubscribe }; let state = mapStateToProps(store ? store.getState() : {}, props); const boundActions = actions ? mapActions(actions, store) : { store }; let update = () => { @@ -38,6 +41,7 @@ export function connect(mapStateToProps, actions) { state = mapped; return this.forceUpdate(); } + emit(); }; this.componentWillReceiveProps = p => { props = p; @@ -49,8 +53,10 @@ export function connect(mapStateToProps, actions) { this.componentWillUnmount = () => { store.unsubscribe(update); }; + this.getChildContext = () => ({ store: subStore }); this.render = () => createElement(Child, assign(assign(assign({}, boundActions), this.props), state)); } + Wrapper.childContextTypes = CONTEXT_TYPES; Wrapper.contextTypes = CONTEXT_TYPES; return (Wrapper.prototype = Object.create(Component.prototype)).constructor = Wrapper; }; diff --git a/src/subscriber.js b/src/subscriber.js new file mode 100644 index 0000000..0517a4d --- /dev/null +++ b/src/subscriber.js @@ -0,0 +1,35 @@ +export default function createSubscriber() { + let listeners = []; + + // Remove a previously-registered listener function. + function unsubscribe(listener) { + let out = []; + for (let i=0; i { unsubscribe(listener); }; + }, + + unsubscribe + }; +} diff --git a/test/preact/preact.test.js b/test/preact/preact.test.js index a0c1ce9..ef3839f 100644 --- a/test/preact/preact.test.js +++ b/test/preact/preact.test.js @@ -211,5 +211,49 @@ describe(`integrations/preact${global.IS_PREACT_8 ? '-8' : ''}`, () => { expect.anything() ); }); + + it('should ignore stale props (zombie children)', async () => { + const orgState = {obj: {foo: 1, bar: 2}, current: 'foo'} + const store = createStore(orgState); + const Child = ({num}) =>
{num}
; + const mapStateToProps = jest.fn((state, {id}) => ({num: state.obj[id]})); + const ConnectedChild = connect(mapStateToProps)(Child); + const Parent = jest.fn(({current}) => ); + const ConnectedParent = connect('current')(Parent); + + render( + + + , + document.body + ); + + expect(mapStateToProps).toHaveBeenCalledTimes(1); + expect(mapStateToProps).toHaveBeenCalledWith( + orgState, {children: NO_CHILDREN, id: 'foo'}); + expect(Parent).toHaveBeenCalledTimes(1); + + mapStateToProps.mockClear(); + Parent.mockClear(); + store.setState({current: 'bar'}); + await sleep(1); + + // Should only call the child mapStateToProps once (thru parent render). + expect(mapStateToProps).toHaveBeenCalledTimes(1); + expect(mapStateToProps).toHaveBeenCalledWith( + {...orgState, current: 'bar'}, {children: NO_CHILDREN, id: 'bar'}); + expect(Parent).toHaveBeenCalledTimes(1); + + mapStateToProps.mockClear(); + Parent.mockClear(); + store.setState({obj: {foo: 1, bar: 3}}); + await sleep(1); + + // Should only call the child mapStateToProps once (ignoring parent render). + expect(mapStateToProps).toHaveBeenCalledTimes(1); + expect(mapStateToProps).toHaveBeenCalledWith( + {obj: {foo: 1, bar: 3}, current: 'bar'}, {children: NO_CHILDREN, id: 'bar'}); + expect(Parent).not.toHaveBeenCalled(); + }); }); }); diff --git a/test/react/react.test.js b/test/react/react.test.js index 00ca647..314b548 100644 --- a/test/react/react.test.js +++ b/test/react/react.test.js @@ -184,4 +184,46 @@ describe('integrations/react', () => { expect.anything() ); }); + + it('should ignore stale props (zombie children)', async () => { + const orgState = {obj: {foo: 1, bar: 2}, current: 'foo'} + const store = createStore(orgState); + const Child = ({num}) =>
{num}
; + const mapStateToProps = jest.fn((state, {id}) => ({num: state.obj[id]})); + const ConnectedChild = connect(mapStateToProps)(Child); + const Parent = jest.fn(({current}) => ); + const ConnectedParent = connect('current')(Parent); + + mount( + + + + ); + + expect(mapStateToProps).toHaveBeenCalledTimes(1); + expect(mapStateToProps).toHaveBeenCalledWith(orgState, {id: 'foo'}); + expect(Parent).toHaveBeenCalledTimes(1); + + mapStateToProps.mockClear(); + Parent.mockClear(); + store.setState({current: 'bar'}); + await sleep(1); + + // Should only call the child mapStateToProps once (thru parent render). + expect(mapStateToProps).toHaveBeenCalledTimes(1); + expect(mapStateToProps).toHaveBeenCalledWith( + {...orgState, current: 'bar'}, {id: 'bar'}); + expect(Parent).toHaveBeenCalledTimes(1); + + mapStateToProps.mockClear(); + Parent.mockClear(); + store.setState({obj: {foo: 1, bar: 3}}); + await sleep(1); + + // Should only call the child mapStateToProps once (ignoring parent render). + expect(mapStateToProps).toHaveBeenCalledTimes(1); + expect(mapStateToProps).toHaveBeenCalledWith( + {obj: {foo: 1, bar: 3}, current: 'bar'}, {id: 'bar'}); + expect(Parent).not.toHaveBeenCalled(); + }); });