From cedbb0bf3fef66666bdd24cabc8fd64c8e0d1b20 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 5 Mar 2018 10:56:18 -0500 Subject: [PATCH] Expanded tests --- .../createComponentWithSubscriptions-test.js | 257 +++++++++++++-- .../src/__tests__/Subscriptions-test.js | 296 ++++++++++++++++++ 2 files changed, 533 insertions(+), 20 deletions(-) create mode 100644 packages/react-dom/src/__tests__/Subscriptions-test.js diff --git a/packages/create-component-with-subscriptions/src/__tests__/createComponentWithSubscriptions-test.js b/packages/create-component-with-subscriptions/src/__tests__/createComponentWithSubscriptions-test.js index aeb8511fcd55e7..9875a3de45cbc0 100644 --- a/packages/create-component-with-subscriptions/src/__tests__/createComponentWithSubscriptions-test.js +++ b/packages/create-component-with-subscriptions/src/__tests__/createComponentWithSubscriptions-test.js @@ -11,6 +11,7 @@ let createComponent; let React; +let ReactNoop; let ReactTestRenderer; describe('CreateComponentWithSubscriptions', () => { @@ -19,11 +20,13 @@ describe('CreateComponentWithSubscriptions', () => { createComponent = require('create-component-with-subscriptions') .createComponent; React = require('react'); + ReactNoop = require('react-noop-renderer'); ReactTestRenderer = require('react-test-renderer'); }); - function createFauxObservable() { - let currentValue; + // Mimics the interface of RxJS `BehaviorSubject` + function createFauxObservable(initialValue) { + let currentValue = initialValue; let subscribedCallback = null; return { getValue: () => currentValue, @@ -49,12 +52,12 @@ describe('CreateComponentWithSubscriptions', () => { it('supports basic subscription pattern', () => { const renderedValues = []; - const Component = createComponent( + const Subscriber = createComponent( { subscribablePropertiesMap: {observable: 'value'}, getDataFor: (subscribable, propertyName) => { expect(propertyName).toBe('observable'); - return observable.getValue(); + return subscribable.getValue(); }, subscribeTo: (valueChangedCallback, subscribable, propertyName) => { expect(propertyName).toBe('observable'); @@ -73,7 +76,7 @@ describe('CreateComponentWithSubscriptions', () => { const observable = createFauxObservable(); const render = ReactTestRenderer.create( - , + , ); // Updates while subscribed should re-render the child component @@ -87,19 +90,14 @@ describe('CreateComponentWithSubscriptions', () => { // Unsetting the subscriber prop should reset subscribed values renderedValues.length = 0; - render.update(); + render.update(); expect(renderedValues).toEqual([undefined]); - - // Updates while unsubscribed should not re-render the child component - renderedValues.length = 0; - observable.update(789); - expect(renderedValues).toEqual([]); }); it('supports multiple subscriptions', () => { const renderedValues = []; - const Component = createComponent( + const Subscriber = createComponent( { subscribablePropertiesMap: { foo: 'foo', @@ -108,9 +106,9 @@ describe('CreateComponentWithSubscriptions', () => { getDataFor: (subscribable, propertyName) => { switch (propertyName) { case 'foo': - return foo.getValue(); + return subscribable.getValue(); case 'bar': - return bar.getValue(); + return subscribable.getValue(); default: throw Error('Unexpected propertyName ' + propertyName); } @@ -118,9 +116,9 @@ describe('CreateComponentWithSubscriptions', () => { subscribeTo: (valueChangedCallback, subscribable, propertyName) => { switch (propertyName) { case 'foo': - return foo.subscribe(valueChangedCallback); + return subscribable.subscribe(valueChangedCallback); case 'bar': - return bar.subscribe(valueChangedCallback); + return subscribable.subscribe(valueChangedCallback); default: throw Error('Unexpected propertyName ' + propertyName); } @@ -144,7 +142,7 @@ describe('CreateComponentWithSubscriptions', () => { const foo = createFauxObservable(); const bar = createFauxObservable(); - const render = ReactTestRenderer.create(); + const render = ReactTestRenderer.create(); // Updates while subscribed should re-render the child component expect(renderedValues).toEqual([{bar: undefined, foo: undefined}]); @@ -160,12 +158,231 @@ describe('CreateComponentWithSubscriptions', () => { // Unsetting the subscriber prop should reset subscribed values renderedValues.length = 0; - render.update(); + render.update(); expect(renderedValues).toEqual([{bar: undefined, foo: undefined}]); + }); + + it('should unsubscribe from old subscribables and subscribe to new subscribables when props change', () => { + const renderedValues = []; + + const Subscriber = createComponent( + { + subscribablePropertiesMap: {observable: 'value'}, + getDataFor: (subscribable, propertyName) => subscribable.getValue(), + subscribeTo: (valueChangedCallback, subscribable, propertyName) => + subscribable.subscribe(valueChangedCallback), + unsubscribeFrom: (subscribable, propertyName, subscription) => + subscription.unsubscribe(), + }, + ({value}) => { + renderedValues.push(value); + return null; + }, + ); + + const observableA = createFauxObservable('a-0'); + const observableB = createFauxObservable('b-0'); + const render = ReactTestRenderer.create( + , + ); + + // Updates while subscribed should re-render the child component + expect(renderedValues).toEqual(['a-0']); + + // Unsetting the subscriber prop should reset subscribed values + renderedValues.length = 0; + render.update(); + expect(renderedValues).toEqual(['b-0']); - // Updates while unsubscribed should not re-render the child component + // Updates to the old subscribable should not re-render the child component renderedValues.length = 0; - foo.update(789); + observableA.update('a-1'); expect(renderedValues).toEqual([]); + + // Updates to the bew subscribable should re-render the child component + observableB.update('b-1'); + expect(renderedValues).toEqual(['b-1']); + }); + + it('should ignore values emitted by a new subscribable until the commit phase', () => { + let parentInstance; + + function Child({value}) { + ReactNoop.yield('Child: ' + value); + return null; + } + + const Subscriber = createComponent( + { + subscribablePropertiesMap: {observable: 'value'}, + getDataFor: (subscribable, propertyName) => subscribable.getValue(), + subscribeTo: (valueChangedCallback, subscribable, propertyName) => + subscribable.subscribe(valueChangedCallback), + unsubscribeFrom: (subscribable, propertyName, subscription) => + subscription.unsubscribe(), + }, + ({value}) => { + ReactNoop.yield('Subscriber: ' + value); + return ; + }, + ); + + class Parent extends React.Component { + state = {}; + + static getDerivedStateFromProps(nextProps, prevState) { + if (nextProps.observable !== prevState.observable) { + return { + observable: nextProps.observable, + }; + } + + return null; + } + + render() { + parentInstance = this; + + return ; + } + } + + const observableA = createFauxObservable('a-0'); + const observableB = createFauxObservable('b-0'); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Subscriber: a-0', 'Child: a-0']); + + // Start React update, but don't finish + ReactNoop.render(); + ReactNoop.flushThrough(['Subscriber: b-0']); + + // Emit some updates from the uncommitted subscribable + observableB.update('b-1'); + observableB.update('b-2'); + observableB.update('b-3'); + + // Mimic a higher-priority interruption + parentInstance.setState({observable: observableA}); + + // Flush everything and ensure that the correct subscribable is used + // We expect the last emitted update to be rendered (because of the commit phase value check) + // But the intermediate ones should be ignored, + // And the final rendered output should be the higher-priority observable. + expect(ReactNoop.flush()).toEqual([ + 'Child: b-0', + 'Subscriber: b-3', + 'Child: b-3', + 'Subscriber: a-0', + 'Child: a-0', + ]); + }); + + it('should not drop values emitted by an old subscribable if the newer subscribable is bailed out on due to an interruption', () => { + let parentInstance; + + function Child({value}) { + ReactNoop.yield('Child: ' + value); + return null; + } + + const Subscriber = createComponent( + { + subscribablePropertiesMap: {observable: 'value'}, + getDataFor: (subscribable, propertyName) => subscribable.getValue(), + subscribeTo: (valueChangedCallback, subscribable, propertyName) => + subscribable.subscribe(valueChangedCallback), + unsubscribeFrom: (subscribable, propertyName, subscription) => + subscription.unsubscribe(), + }, + ({value}) => { + ReactNoop.yield('Subscriber: ' + value); + return ; + }, + ); + + class Parent extends React.Component { + state = {}; + + static getDerivedStateFromProps(nextProps, prevState) { + if (nextProps.observable !== prevState.observable) { + return { + observable: nextProps.observable, + }; + } + + return null; + } + + render() { + parentInstance = this; + + return ; + } + } + + const observableA = createFauxObservable('a-0'); + const observableB = createFauxObservable('b-0'); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Subscriber: a-0', 'Child: a-0']); + + // Start React update, but don't finish + ReactNoop.render(); + ReactNoop.flushThrough(['Subscriber: b-0']); + + // Emit some updates from the old subscribable + observableA.update('a-1'); + observableA.update('a-2'); + + // Mimic a higher-priority interruption + parentInstance.setState({observable: observableA}); + + // Flush everything and ensure that the correct subscribable is used + // We expect the new subscribable to finish rendering, + // But then the updated values from the old subscribable should be used. + expect(ReactNoop.flush()).toEqual([ + 'Child: b-0', + 'Subscriber: a-2', + 'Child: a-2', + ]); + + // Updates from the new subsribable should be ignored. + observableB.update('b-1'); + ReactNoop.flush(); + }); + + it('should pass all non-subscribable props through to the child component', () => { + const renderedValues = []; + + const Subscriber = createComponent( + { + subscribablePropertiesMap: {observable: 'value'}, + getDataFor: (subscribable, propertyName) => subscribable.getValue(), + subscribeTo: (valueChangedCallback, subscribable, propertyName) => + subscribable.subscribe(valueChangedCallback), + unsubscribeFrom: (subscribable, propertyName, subscription) => + subscription.unsubscribe(), + }, + ({bar, foo, value}) => { + renderedValues.push({bar, foo, value}); + return null; + }, + ); + + const observable = createFauxObservable(true); + const render = ReactTestRenderer.create( + , + ); + + expect(renderedValues).toEqual([ + { + bar: 'abc', + foo: 123, + value: true, + }, + ]); }); + + // TODO Decide how to test missing/undefined subscribables }); diff --git a/packages/react-dom/src/__tests__/Subscriptions-test.js b/packages/react-dom/src/__tests__/Subscriptions-test.js new file mode 100644 index 00000000000000..7804e3c8a457d2 --- /dev/null +++ b/packages/react-dom/src/__tests__/Subscriptions-test.js @@ -0,0 +1,296 @@ +let React; +let ReactFeatureFlags; +let ReactNoop; +let PropTypes; + +describe('Subscriptions', () => { + beforeEach(() => { + jest.resetModules(); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + React = require('react'); + ReactNoop = require('react-noop-renderer'); + PropTypes = require('prop-types'); + }); + + xit('should work', () => { + let parentInstance; + + function Child(props) { + ReactNoop.yield('Child: ' + props.value); + return null; + } + + class Parent extends React.Component { + state = {}; + + static getDerivedStateFromProps(nextProps, prevState) { + if (nextProps.dataSource !== prevState.dataSource) { + return { + dataSource: nextProps.dataSource, + }; + } + + return null; + } + + render() { + parentInstance = this; + + return ; + } + } + + class Subscriber extends React.Component { + state = {}; + + static getDerivedStateFromProps(nextProps, prevState) { + if (nextProps.dataSource !== prevState.dataSource) { + return { + data: nextProps.dataSource.value, + dataSource: nextProps.dataSource, + }; + } + + return null; + } + + shouldComponentUpdate(nextProps, nextState) { + if (this.state.data !== nextState.data) { + return true; + } + return false; + } + + componentDidMount() { + this.finalizeSubscription(); + } + + componentDidUpdate(prevProps, prevState) { + if (this.state.dataSource !== prevState.dataSource) { + prevState.dataSource.unsubscribe(); + this.finalizeSubscription(); + } + } + + render() { + ReactNoop.yield(`Subscriber: ${this.state.data}`); + return ; + } + + finalizeSubscription() { + this.state.dataSource.subscribe(this.handleUpdate); + + const data = this.state.dataSource.value; + if (data !== this.state.data) { + this.setState({data}); + } + } + + handleUpdate = dataSource => { + if (this.props.compareDataSource) { + this.setState(state => { + if (dataSource === state.dataSource) { + return { + data: dataSource.value, + }; + } + }); + } else { + this.setState(state => { + return { + data: state.dataSource.value, + }; + }); + } + }; + } + + class Subscribable { + constructor(value) { + this.value = value; + } + + update(value) { + this.value = value; + + if (typeof this.callback === 'function') { + this.callback(this); + } + } + + subscribe(callback) { + this.callback = callback; + } + + unsubscribe(callback) { + this.callback = null; + } + } + + const dataSourceA = new Subscribable('A-0'); + const dataSourceB = new Subscribable('B-0'); + const dataSourceC = new Subscribable('C-0'); + const dataSourceD = new Subscribable('D-0'); + const dataSourceE = new Subscribable('E-0'); + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Subscriber: A-0', 'Child: A-0']); + + // Test events dispatched by stale subscribables + // These test ensure that stale subscribables don't impact state. + + ReactNoop.render(); + ReactNoop.flushThrough(['Subscriber: B-0']); + dataSourceA.update('A-1'); // Ignore old data source + expect(ReactNoop.flush()).toEqual(['Child: B-0']); + + ReactNoop.render(); + ReactNoop.flushThrough(['Subscriber: C-0']); + dataSourceB.update('B-1'); // Ignore old data source + expect(ReactNoop.flush()).toEqual(['Child: C-0']); + + dataSourceB.update('B-2'); + dataSourceC.update('C-1'); + expect(ReactNoop.flush()).toEqual(['Subscriber: C-1', 'Child: C-1']); + + // Test events dispatched by new subscribable if pending changes are discarded + // These test ensure that new subscribables don't impact state if they are abandoned. + + ReactNoop.render(); + ReactNoop.flushThrough(['Subscriber: D-0']); // Start update + dataSourceC.update('C-2'); + dataSourceD.update('D-1'); + parentInstance.setState({dataSource: dataSourceC}); // Override previous update + expect(ReactNoop.flush()).toEqual([ + 'Child: D-0', + 'Subscriber: D-1', + 'Child: D-1', + 'Subscriber: C-2', + 'Child: C-2', + ]); + + ReactNoop.render(); + ReactNoop.flushThrough(['Subscriber: E-0']); // Start update + dataSourceC.update('C-3'); + dataSourceD.update('D-2'); + dataSourceE.update('E-1'); + dataSourceD.update('D-3'); + dataSourceC.update('C-4'); + dataSourceE.update('E-2'); + parentInstance.setState({dataSource: dataSourceD}); // Override previous update + expect(ReactNoop.flush()).toEqual([ + 'Child: E-0', + 'Subscriber: E-2', + 'Child: E-2', + 'Subscriber: D-3', + 'Child: D-3', + ]); + + dataSourceC.update('C-5'); + dataSourceD.update('D-4'); + dataSourceE.update('E-3'); + expect(ReactNoop.flush()).toEqual(['Subscriber: D-4', 'Child: D-4']); + }); + + it('should work', () => { + let parentInstance; + + class ChildOne extends React.Component { + static contextTypes = { + wrapper: PropTypes.any, + }; + + render() { + ReactNoop.yield('ChildOne: ' + this.context.wrapper.value); + return null; + } + } + + class ChildTwo extends React.Component { + static contextTypes = { + wrapper: PropTypes.any, + }; + + render() { + ReactNoop.yield('ChildTwo: ' + this.context.wrapper.value); + return null; + } + } + + class Example extends React.Component { + state = { + wrapper: {}, + }; + + static childContextTypes = { + wrapper: PropTypes.any, + }; + + static getDerivedStateFromProps(nextProps, prevState) { + if (nextProps.value !== prevState.value) { + // NOTE: Uncomment to show bug: + // prevState.wrapper.value = nextProps.value; + + // Note that if we mutate 'prevState.wrapper.value' here, + // Subsequent changes to state that aren't props-initiated won't be reflected. + // NOTE: Comment-out to show bug: + return { + wrapperValue: nextProps.value, + }; + } + + return null; + } + + getChildContext() { + return { + wrapper: this.state.wrapper, + }; + } + + render() { + parentInstance = this; + + // (Intentionally mutate to mimic Relay context.) + // Mutating here ensures props-updates and state-updates are both reflected. + // NOTE: Comment-out to show bug: + this.state.wrapper.value = this.state.wrapperValue; + + ReactNoop.yield(`Example: ${this.state.wrapper.value}`); + + return ( + + + + + ); + } + + simulateMergeMapMutation(value) { + // NOTE: Uncomment to show bug: + //this.state.wrapper.value = value; + + // NOTE: Comment-out to show bug: + this.setState({wrapperValue: value}); + } + } + + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual([ + 'Example: foo', + 'ChildOne: foo', + 'ChildTwo: foo', + ]); + + ReactNoop.render(); + ReactNoop.flushThrough(['Example: bar', 'ChildOne: bar']); // Start update + parentInstance.simulateMergeMapMutation('baz'); // Simulate another event that might set conflicting state + expect(ReactNoop.flush()).toEqual([ + 'ChildTwo: bar', + 'Example: baz', + 'ChildOne: baz', + 'ChildTwo: baz', + ]); + }); +});