From 87f7c31c166a444900f616b617de73cb351881b4 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 5 Mar 2019 22:10:59 -0800 Subject: [PATCH] Updated useSubscription API to require deps array --- .../useSubscription-test.internal.js | 266 +++++++++--------- packages/react-hooks/src/useSubscription.js | 72 +++-- 2 files changed, 178 insertions(+), 160 deletions(-) diff --git a/packages/react-hooks/src/__tests__/useSubscription-test.internal.js b/packages/react-hooks/src/__tests__/useSubscription-test.internal.js index 3027a6c4c4466..6f3ed4225d1ca 100644 --- a/packages/react-hooks/src/__tests__/useSubscription-test.internal.js +++ b/packages/react-hooks/src/__tests__/useSubscription-test.internal.js @@ -49,34 +49,29 @@ describe('useSubscription', () => { return replaySubject; } - // Mimic createSubscription API to simplify testing - function createSubscription({getCurrentValue, subscribe}) { - return function Subscription({children, source}) { + it('supports basic subscription pattern', () => { + function Subscription({source}) { const value = useSubscription( - React.useMemo(() => ({source, getCurrentValue, subscribe}), [source]), + () => ({ + getCurrentValue: () => source.getValue(), + subscribe: callback => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }), + [source], ); + return ; + } - return React.createElement(children, {value}); - }; - } - - it('supports basic subscription pattern', () => { - const Subscription = createSubscription({ - getCurrentValue: source => source.getValue(), - subscribe: (source, callback) => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe(); - }, - }); + function Child({value = 'default'}) { + Scheduler.yieldValue(value); + return null; + } const observable = createBehaviorSubject(); const renderer = ReactTestRenderer.create( - - {({value = 'default'}) => { - Scheduler.yieldValue(value); - return null; - }} - , + , {unstable_isConcurrent: true}, ); @@ -94,30 +89,36 @@ describe('useSubscription', () => { }); it('should support observable types like RxJS ReplaySubject', () => { - const Subscription = createSubscription({ - getCurrentValue: source => { - let currentValue; - source - .subscribe(value => { - currentValue = value; - }) - .unsubscribe(); - return currentValue; - }, - subscribe: (source, callback) => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe; - }, - }); + function Subscription({source}) { + const value = useSubscription( + () => ({ + getCurrentValue: () => { + let currentValue; + source + .subscribe(tempValue => { + currentValue = tempValue; + }) + .unsubscribe(); + return currentValue; + }, + subscribe: callback => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }), + [source], + ); + return ; + } - function render({value = 'default'}) { + function Child({value = 'default'}) { Scheduler.yieldValue(value); return null; } let observable = createReplaySubject('initial'); const renderer = ReactTestRenderer.create( - {render}, + , {unstable_isConcurrent: true}, ); expect(Scheduler).toFlushAndYield(['initial']); @@ -128,20 +129,26 @@ describe('useSubscription', () => { // Unsetting the subscriber prop should reset subscribed values observable = createReplaySubject(undefined); - renderer.update({render}); + renderer.update(); expect(Scheduler).toFlushAndYield(['default']); }); it('should unsubscribe from old subscribables and subscribe to new subscribables when props change', () => { - const Subscription = createSubscription({ - getCurrentValue: source => source.getValue(), - subscribe: (source, callback) => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe(); - }, - }); + function Subscription({source}) { + const value = useSubscription( + () => ({ + getCurrentValue: () => source.getValue(), + subscribe: callback => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }), + [source], + ); + return ; + } - function render({value = 'default'}) { + function Child({value = 'default'}) { Scheduler.yieldValue(value); return null; } @@ -150,7 +157,7 @@ describe('useSubscription', () => { const observableB = createBehaviorSubject('b-0'); const renderer = ReactTestRenderer.create( - {render}, + , {unstable_isConcurrent: true}, ); @@ -158,7 +165,7 @@ describe('useSubscription', () => { expect(Scheduler).toFlushAndYield(['a-0']); // Unsetting the subscriber prop should reset subscribed values - renderer.update({render}); + renderer.update(); expect(Scheduler).toFlushAndYield(['b-0']); // Updates to the old subscribable should not re-render the child component @@ -173,32 +180,31 @@ describe('useSubscription', () => { it('should ignore values emitted by a new subscribable until the commit phase', () => { const log = []; - function Child({value}) { - Scheduler.yieldValue('Child: ' + value); - return null; + function Subscription({source}) { + const value = useSubscription( + () => ({ + getCurrentValue: () => source.getValue(), + subscribe: callback => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }), + [source], + ); + return ; } - const Subscription = createSubscription({ - getCurrentValue: source => source.getValue(), - subscribe: (source, callback) => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe(); - }, - }); - - class Parent extends React.Component { - state = {}; - - static getDerivedStateFromProps(nextProps, prevState) { - if (nextProps.observed !== prevState.observed) { - return { - observed: nextProps.observed, - }; - } + function Outer({value}) { + Scheduler.yieldValue('Outer: ' + value); + return ; + } - return null; - } + function Inner({value}) { + Scheduler.yieldValue('Inner: ' + value); + return null; + } + class Parent extends React.Component { componentDidMount() { log.push('Parent.componentDidMount'); } @@ -208,14 +214,7 @@ describe('useSubscription', () => { } render() { - return ( - - {({value = 'default'}) => { - Scheduler.yieldValue('Subscriber: ' + value); - return ; - }} - - ); + return ; } } @@ -226,12 +225,12 @@ describe('useSubscription', () => { , {unstable_isConcurrent: true}, ); - expect(Scheduler).toFlushAndYield(['Subscriber: a-0', 'Child: a-0']); + expect(Scheduler).toFlushAndYield(['Outer: a-0', 'Inner: a-0']); expect(log).toEqual(['Parent.componentDidMount']); // Start React update, but don't finish renderer.update(); - expect(Scheduler).toFlushAndYieldThrough(['Subscriber: b-0']); + expect(Scheduler).toFlushAndYieldThrough(['Outer: b-0']); expect(log).toEqual(['Parent.componentDidMount']); // Emit some updates from the uncommitted subscribable @@ -247,11 +246,11 @@ describe('useSubscription', () => { // But the intermediate ones should be ignored, // And the final rendered output should be the higher-priority observable. expect(Scheduler).toFlushAndYield([ - 'Child: b-0', - 'Subscriber: b-3', - 'Child: b-3', - 'Subscriber: a-0', - 'Child: a-0', + 'Inner: b-0', + 'Outer: b-3', + 'Inner: b-3', + 'Outer: a-0', + 'Inner: a-0', ]); expect(log).toEqual([ 'Parent.componentDidMount', @@ -263,32 +262,31 @@ describe('useSubscription', () => { it('should not drop values emitted between updates', () => { const log = []; - function Child({value}) { - Scheduler.yieldValue('Child: ' + value); - return null; + function Subscription({source}) { + const value = useSubscription( + () => ({ + getCurrentValue: () => source.getValue(), + subscribe: callback => { + const subscription = source.subscribe(callback); + return () => subscription.unsubscribe(); + }, + }), + [source], + ); + return ; } - const Subscription = createSubscription({ - getCurrentValue: source => source.getValue(), - subscribe: (source, callback) => { - const subscription = source.subscribe(callback); - return () => subscription.unsubscribe(); - }, - }); - - class Parent extends React.Component { - state = {}; - - static getDerivedStateFromProps(nextProps, prevState) { - if (nextProps.observed !== prevState.observed) { - return { - observed: nextProps.observed, - }; - } + function Outer({value}) { + Scheduler.yieldValue('Outer: ' + value); + return ; + } - return null; - } + function Inner({value}) { + Scheduler.yieldValue('Inner: ' + value); + return null; + } + class Parent extends React.Component { componentDidMount() { log.push('Parent.componentDidMount'); } @@ -298,14 +296,7 @@ describe('useSubscription', () => { } render() { - return ( - - {({value = 'default'}) => { - Scheduler.yieldValue('Subscriber: ' + value); - return ; - }} - - ); + return ; } } @@ -316,12 +307,12 @@ describe('useSubscription', () => { , {unstable_isConcurrent: true}, ); - expect(Scheduler).toFlushAndYield(['Subscriber: a-0', 'Child: a-0']); + expect(Scheduler).toFlushAndYield(['Outer: a-0', 'Inner: a-0']); expect(log).toEqual(['Parent.componentDidMount']); // Start React update, but don't finish renderer.update(); - expect(Scheduler).toFlushAndYieldThrough(['Subscriber: b-0']); + expect(Scheduler).toFlushAndYieldThrough(['Outer: b-0']); expect(log).toEqual(['Parent.componentDidMount']); // Emit some updates from the old subscribable @@ -335,9 +326,9 @@ describe('useSubscription', () => { // We expect the new subscribable to finish rendering, // But then the updated values from the old subscribable should be used. expect(Scheduler).toFlushAndYield([ - 'Child: b-0', - 'Subscriber: a-2', - 'Child: a-2', + 'Inner: b-0', + 'Outer: a-2', + 'Inner: a-2', ]); expect(log).toEqual([ 'Parent.componentDidMount', @@ -356,12 +347,24 @@ describe('useSubscription', () => { }); it('should guard against updates that happen after unmounting', () => { - const Subscription = createSubscription({ - getCurrentValue: source => source.getValue(), - subscribe: (source, callback) => { - return source.subscribe(callback); - }, - }); + function Subscription({source}) { + const value = useSubscription( + () => ({ + getCurrentValue: () => source.getValue(), + subscribe: callback => { + const unsubscribe = source.subscribe(callback); + return () => unsubscribe(); + }, + }), + [source], + ); + return ; + } + + function Child({value}) { + Scheduler.yieldValue(value); + return null; + } const eventHandler = { _callbacks: [], @@ -393,12 +396,7 @@ describe('useSubscription', () => { }); const renderer = ReactTestRenderer.create( - - {({value}) => { - Scheduler.yieldValue(value); - return null; - }} - , + , {unstable_isConcurrent: true}, ); diff --git a/packages/react-hooks/src/useSubscription.js b/packages/react-hooks/src/useSubscription.js index ee4078fe2c4c9..c749203cbaa81 100644 --- a/packages/react-hooks/src/useSubscription.js +++ b/packages/react-hooks/src/useSubscription.js @@ -1,30 +1,49 @@ -import {useEffect, useState} from 'react'; +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ -export function useSubscription({ - // This is the thing being subscribed to (e.g. an observable, event dispatcher, etc). - source, +import {useEffect, useMemo, useState} from 'react'; - // (Synchronously) returns the current value of our subscription source. - getCurrentValue, +// Hook used for safely managing subscriptions in concurrent mode. +// It requires two parameters: a factory function and a dependencies array. +export function useSubscription( + // This function is called whenever the specified dependencies change. + // It should return an object with two keys (functions) documented below. + nextCreate: () => {| + // Get the current subscription value. + getCurrentValue: () => T, - // This function is passed an event handler to attach to the subscription source. - // It should return an unsubscribe function that removes the handler. - subscribe, -}) { - // Read the current value from our subscription source. + // This function is passed a callback to be called any time the subscription changes. + // It should return an unsubscribe function. + subscribe: (() => void) => () => void, + |}, + // Dependencies array. + // Any time one of the inputs change, a new subscription will be setup, + // and the previous listener will be unsubscribed. + deps: Array, +) { + const current = useMemo(nextCreate, deps); + + // Read the current subscription value. // When this value changes, we'll schedule an update with React. - // It's important to also store the source itself so that we can check for staleness. + // It's important to also store the current inputs as well so we can check for staleness. // (See the comment in checkForUpdates() below for more info.) const [state, setState] = useState({ - source, - value: getCurrentValue(source), + current, + value: current.getCurrentValue(), }); - // If the source has changed since our last render, schedule an update with its current value. - if (state.source !== source) { + // If the inputs have changed since our last render, schedule an update with the current value. + // We could do this in our effect handler below but there's no need to wait in this case. + if (state.current !== current) { setState({ - source, - value: getCurrentValue(source), + current, + value: current.getCurrentValue(), }); } @@ -51,18 +70,18 @@ export function useSubscription({ } setState(prevState => { - // Ignore values from stale sources! + // Ignore values from stale subscriptions! // Since we subscribe an unsubscribe in a passive effect, - // it's possible that this callback will be invoked for a stale (previous) source. - // This check avoids scheduling an update for htat stale source. - if (prevState.source !== source) { + // it's possible that this callback will be invoked for a stale (previous) subscription. + // This check avoids scheduling an update for the stale subscription. + if (prevState.current !== current) { return prevState; } - // Some subscription sources will auto-invoke the handler, even if the value hasn't changed. + // Some subscriptions will auto-invoke the handler when it's attached. // If the value hasn't changed, no update is needed. // Return state as-is so React can bail out and avoid an unnecessary render. - const value = getCurrentValue(source); + const value = current.getCurrentValue(); if (prevState.value === value) { return prevState; } @@ -70,7 +89,8 @@ export function useSubscription({ return {...prevState, value}; }); }; - const unsubscribe = subscribe(source, checkForUpdates); + + const unsubscribe = current.subscribe(checkForUpdates); // Because we're subscribing in a passive effect, // it's possible that an update has occurred between render and our effect handler. @@ -82,7 +102,7 @@ export function useSubscription({ unsubscribe(); }; }, - [getCurrentValue, source, subscribe], + [current], ); // Return the current value for our caller to use while rendering.