From 3a0e79b00a919161272162f1e05e0b4d86012bd5 Mon Sep 17 00:00:00 2001 From: Ostap Chervak Date: Wed, 7 Apr 2021 22:30:26 +0300 Subject: [PATCH 1/5] Move to LiftWrapperII. Fix waste re-render --- packages/focal/src/react/intrinsic.ts | 6 +- packages/focal/src/react/react.ts | 133 ++++++++++++++------------ 2 files changed, 73 insertions(+), 66 deletions(-) diff --git a/packages/focal/src/react/intrinsic.ts b/packages/focal/src/react/intrinsic.ts index 3722c99..644ecba 100644 --- a/packages/focal/src/react/intrinsic.ts +++ b/packages/focal/src/react/intrinsic.ts @@ -5,7 +5,7 @@ */ import * as React from 'react' import { ObservableReactHTMLProps, ObservableReactChildren } from './observablePropTypes' -import { LiftWrapperProps, LiftWrapper } from './react' +import { LiftWrapperProps, LiftWrapperII } from './react' export interface LiftedIntrinsicComponentProps extends ObservableReactHTMLProps { mount?: React.Ref @@ -26,7 +26,7 @@ export function liftIntrinsic< ): LiftedIntrinsic { return (props: LiftedIntrinsicComponentProps) => React.createElement>>( - LiftWrapper, + LiftWrapperII, { component: intrinsicClassName, props: props } ) } @@ -79,7 +79,7 @@ export function createLiftedIntrinsics(): LiftedIntrinsics { ) r.Fragment = (props: LiftedFragmentAttributes) => - React.createElement(LiftWrapper, { component: React.Fragment, props }) + React.createElement(LiftWrapperII, { component: React.Fragment, props }) return r as LiftedIntrinsics } diff --git a/packages/focal/src/react/react.ts b/packages/focal/src/react/react.ts index adce812..b53fde3 100644 --- a/packages/focal/src/react/react.ts +++ b/packages/focal/src/react/react.ts @@ -33,41 +33,51 @@ export interface LiftWrapperState { subscription?: Subscription | null } -/** - * A wrapper component that allows to use observables for prop values of a - * given component. - */ -export class LiftWrapper - extends React.Component, LiftWrapperState> { - state = LiftWrapper._initState - - static _initState: LiftWrapperState = { - renderCache: null, - subscription: null - } - - static _endState: LiftWrapperState = { - subscription: null - } +function useForceUpdate(): () => void { + return React.useReducer(() => ({}), {})[1] as () => void +} - render() { - return this.state.renderCache || null - } +export const LiftWrapperII = (cProps: LiftWrapperProps) => { + const subscription = React.useRef(null) + const renderCache = React.useRef(null) + const forceUpdate = useForceUpdate() - private _subscribe(newProps: LiftWrapperProps) { + const _subscribe = (newProps: LiftWrapperProps) => { const { props, component } = newProps let n = 0 // eslint-disable-next-line @typescript-eslint/no-use-before-define walkObservables(props, () => n += 1) + const state = { subscription: subscription.current, renderCache: renderCache.current } + + let inSync = false + + const setState = (s: (LiftWrapperState | ((s: LiftWrapperState) => LiftWrapperState))) => { + const newState = typeof s === 'function' ? s(state) : s + + if ('subscription' in newState) { + subscription.current = newState.subscription + } + + if ('renderCache' in newState) { + if (renderCache.current === newState.renderCache) { + return + } + + renderCache.current = newState.renderCache + + if (inSync) { + forceUpdate() + } + } + } + switch (n) { case 0: - this.setState({ - subscription: null, - // eslint-disable-next-line @typescript-eslint/no-use-before-define - renderCache: render(component, props) - }) + subscription.current = null + // eslint-disable-next-line @typescript-eslint/no-use-before-define + renderCache.current = render(component, props) break // @NOTE original Calmm code below @@ -76,44 +86,37 @@ export class LiftWrapper // Could this be replaced by a regular closure? Perhaps using // a class is an optimization? case 1: - new RenderOne(this, newProps) // eslint-disable-line + new RenderOne({ props: newProps, state, setState }, newProps) // eslint-disable-line break default: - new RenderMany(this, newProps, n) // eslint-disable-line + new RenderMany({ props: newProps, state, setState }, newProps, n) // eslint-disable-line break } - } - private _unsubscribe() { - const subscription = this.state ? this.state.subscription : null - if (subscription) - subscription.unsubscribe() + inSync = true } - // eslint-disable-next-line camelcase - UNSAFE_componentWillReceiveProps(newProps: LiftWrapperProps) { - this._unsubscribe() - this._subscribe(newProps) + const _unsubscribe = () => { + if (subscription.current) { + subscription.current.unsubscribe() + } } - // eslint-disable-next-line camelcase - UNSAFE_componentWillMount() { - this._unsubscribe() - this._subscribe(this.props) - } + React.useEffect(() => _unsubscribe, []) - componentWillUnmount() { - this._unsubscribe() - this.setState(LiftWrapper._initState) - } + _unsubscribe() + _subscribe(cProps) - shouldComponentUpdate( - _newProps: Readonly>, - newState: Readonly, - _newContext: any - ) { - return newState.renderCache !== this.state.renderCache - } + return renderCache.current || null +} + +LiftWrapperII._initState = { + renderCache: null, + subscription: null +} + +LiftWrapperII._endState = { + subscription: null } // here we only say TProps, but a lifted component @@ -155,7 +158,7 @@ export function lift( ) { return (props: LiftedComponentProps) => React.createElement>( - LiftWrapper, + LiftWrapperII, { component: component, props: props } ) } @@ -349,13 +352,19 @@ function warnEmptyObservable(componentName: string | undefined) { * * @template P component props */ +interface LiftWrapperType

{ + state: LiftWrapperState + props: LiftWrapperProps

+ setState(state: (LiftWrapperState | ((state: LiftWrapperState) => LiftWrapperState))): void +} + class RenderOne

implements Subscription { - private _liftedComponent: LiftWrapper

+ private _liftedComponent: LiftWrapperType

private _innerSubscription: RxSubscription | null = null private _receivedValue = false constructor( - liftedComponent: LiftWrapper

, + liftedComponent: LiftWrapperType

, newProps: LiftWrapperProps

) { const state: LiftWrapperState = { @@ -363,8 +372,7 @@ class RenderOne

implements Subscription { renderCache: liftedComponent.state && liftedComponent.state.renderCache } - this._liftedComponent = - new FakeComponent

(state, newProps) as LiftWrapper

+ this._liftedComponent = new FakeComponent

(state, newProps) walkObservables( newProps.props, @@ -406,7 +414,7 @@ class RenderOne

implements Subscription { private _handleCompleted() { this._innerSubscription = null - this._liftedComponent.setState(LiftWrapper._endState) + this._liftedComponent.setState(LiftWrapperII._endState) } } @@ -416,12 +424,12 @@ class RenderOne

implements Subscription { * @template P component props */ class RenderMany

implements Subscription { - private _liftedComponent: LiftWrapper

+ private _liftedComponent: LiftWrapperType

private _values: any[] private _innerSubscriptions: (RxSubscription | null)[] constructor( - liftedComponent: LiftWrapper

, + liftedComponent: LiftWrapperType

, newProps: LiftWrapperProps

, N: number ) { @@ -430,8 +438,7 @@ class RenderMany

implements Subscription { renderCache: liftedComponent.state && liftedComponent.state.renderCache } - this._liftedComponent = - new FakeComponent(state, newProps) as LiftWrapper

+ this._liftedComponent = new FakeComponent(state, newProps) this._innerSubscriptions = [] this._values = Array(N) @@ -514,7 +521,7 @@ class RenderMany

implements Subscription { if (this._innerSubscriptions[i]) return - this._liftedComponent.setState(LiftWrapper._endState) + this._liftedComponent.setState(LiftWrapperII._endState) } } From 63aa962e5f4f8ded16c7f8457dca3a1e7bc48f2b Mon Sep 17 00:00:00 2001 From: Ostap Chervak Date: Thu, 8 Apr 2021 15:54:04 +0300 Subject: [PATCH 2/5] Refactor to namespace. Reduce 1 closure creation --- packages/focal/src/react/intrinsic.ts | 12 +- packages/focal/src/react/react.ts | 172 ++++++++++++++------------ 2 files changed, 97 insertions(+), 87 deletions(-) diff --git a/packages/focal/src/react/intrinsic.ts b/packages/focal/src/react/intrinsic.ts index 644ecba..151d5f8 100644 --- a/packages/focal/src/react/intrinsic.ts +++ b/packages/focal/src/react/intrinsic.ts @@ -5,7 +5,7 @@ */ import * as React from 'react' import { ObservableReactHTMLProps, ObservableReactChildren } from './observablePropTypes' -import { LiftWrapperProps, LiftWrapperII } from './react' +import { LiftWrapper } from './react' export interface LiftedIntrinsicComponentProps extends ObservableReactHTMLProps { mount?: React.Ref @@ -15,7 +15,7 @@ export interface LiftedIntrinsicComponentProps extends ObservableReactHTMLPro export interface LiftedIntrinsic< E extends Element, A extends React.HTMLAttributes = React.AllHTMLAttributes> { (props: LiftedIntrinsicComponentProps): - React.ReactElement>> + React.ReactElement>> } export function liftIntrinsic< @@ -25,8 +25,8 @@ export function liftIntrinsic< intrinsicClassName: K ): LiftedIntrinsic { return (props: LiftedIntrinsicComponentProps) => - React.createElement>>( - LiftWrapperII, + React.createElement>>( + LiftWrapper.Renderer, { component: intrinsicClassName, props: props } ) } @@ -45,7 +45,7 @@ export interface LiftedFragmentAttributes extends ObservableReactChildren, React export interface LiftedFragment { (props: LiftedFragmentAttributes): // @TODO this probably isn't a correct type for it - React.ReactElement>> + React.ReactElement>> } interface ExtraLiftedIntrinsics { @@ -79,7 +79,7 @@ export function createLiftedIntrinsics(): LiftedIntrinsics { ) r.Fragment = (props: LiftedFragmentAttributes) => - React.createElement(LiftWrapperII, { component: React.Fragment, props }) + React.createElement(LiftWrapper.Renderer, { component: React.Fragment, props }) return r as LiftedIntrinsics } diff --git a/packages/focal/src/react/react.ts b/packages/focal/src/react/react.ts index b53fde3..5dc78e7 100644 --- a/packages/focal/src/react/react.ts +++ b/packages/focal/src/react/react.ts @@ -15,34 +15,49 @@ export interface Subscription { unsubscribe(): void } -export type Lifted = { - [K in keyof T]: T[K] | Observable -} +export namespace LiftWrapper { + export type Lifted = { + [K in keyof T]: T[K] | Observable + } -export interface LiftWrapperProps { - component: React.Component - | React.StatelessComponent + export interface Props { + component: React.Component + | React.FunctionComponent | React.ComponentClass | React.ComponentType | keyof React.ReactHTML - props: Lifted -} + props: Lifted + } -export interface LiftWrapperState { - renderCache?: React.DOMElement | null - subscription?: Subscription | null -} + type RenderCache = React.DOMElement | null + type WrapperSubscription = Subscription | null -function useForceUpdate(): () => void { - return React.useReducer(() => ({}), {})[1] as () => void -} + export interface State { + renderCache?: RenderCache + subscription?: WrapperSubscription + } -export const LiftWrapperII = (cProps: LiftWrapperProps) => { - const subscription = React.useRef(null) - const renderCache = React.useRef(null) - const forceUpdate = useForceUpdate() + export interface Type

{ + state: State + props: Props

+ setState(state: (State | ((state: State) => State))): void + } + function useForceUpdate(): () => void { + return React.useReducer(() => ({}), {})[1] as () => void + } + + const unsubscribe = (subscription: React.MutableRefObject) => { + if (subscription.current) { + subscription.current.unsubscribe() + } + } - const _subscribe = (newProps: LiftWrapperProps) => { + const useSubscribe = ( + newProps: Props, + subscription: React.MutableRefObject, + renderCache: React.MutableRefObject + ) => { + const forceUpdate = useForceUpdate() const { props, component } = newProps let n = 0 @@ -53,14 +68,14 @@ export const LiftWrapperII = (cProps: LiftWrapperProps) => { let inSync = false - const setState = (s: (LiftWrapperState | ((s: LiftWrapperState) => LiftWrapperState))) => { + const setState = (s: (State | ((s: State) => State))) => { const newState = typeof s === 'function' ? s(state) : s - if ('subscription' in newState) { + if (newState.subscription) { subscription.current = newState.subscription } - if ('renderCache' in newState) { + if (newState.renderCache) { if (renderCache.current === newState.renderCache) { return } @@ -96,33 +111,54 @@ export const LiftWrapperII = (cProps: LiftWrapperProps) => { inSync = true } - const _unsubscribe = () => { - if (subscription.current) { - subscription.current.unsubscribe() - } +/** + * A dummy React component mock that is used to preserve any state changes + * pushed onto the component. + */ +export class FakeRenderer

implements LiftWrapper.Type

{ + constructor( + public state: LiftWrapper.State, + public props: LiftWrapper.Props

+ ) {} + + setState(state: (LiftWrapper.State | ((state: LiftWrapper.State) => LiftWrapper.State))) { + const newState = typeof state === 'function' ? state(this.state) : state + + if ('subscription' in newState) + this.state.subscription = newState.subscription + if ('renderCache' in newState) + this.state.renderCache = newState.renderCache } +} - React.useEffect(() => _unsubscribe, []) + export const initState = { + renderCache: null, + subscription: null + } - _unsubscribe() - _subscribe(cProps) + export const endState = { + subscription: null + } - return renderCache.current || null -} + export const Renderer = (props: Props) => { + const subscription = React.useRef(initState.subscription) + const renderCache = React.useRef(initState.renderCache) -LiftWrapperII._initState = { - renderCache: null, - subscription: null -} + React.useEffect(() => () => unsubscribe(subscription), []) -LiftWrapperII._endState = { - subscription: null + unsubscribe(subscription) + useSubscribe(props, subscription, renderCache) + + return renderCache.current || null + } + + Renderer.displayName = 'LiftWrapper' } // here we only say TProps, but a lifted component // will also accept a value of Observable for any prop of // type T. -export type LiftedComponentProps = Lifted & { +export type LiftedComponentProps = LiftWrapper.Lifted & { mount?: React.Ref } @@ -157,8 +193,8 @@ export function lift( component: React.ComponentClass | React.StatelessComponent ) { return (props: LiftedComponentProps) => - React.createElement>( - LiftWrapperII, + React.createElement>( + LiftWrapper.Renderer, { component: component, props: props } ) } @@ -178,7 +214,7 @@ const PROP_REF = 'ref' * @param action action to run for each observable prop */ function walkObservables( - props: Lifted, + props: LiftWrapper.Lifted, action: (obs: Observable) => void ) { for (const key in props) { @@ -223,7 +259,7 @@ function render

( | React.ComponentClass

| React.ComponentType | keyof React.ReactHTML, - props: Lifted

, + props: LiftWrapper.Lifted

, observedValues: any[] = [] ): React.DOMElement { // @TODO can we do a better type here? @@ -312,26 +348,6 @@ function render

( ) } -/** - * A dummy React component mock that is used to preserve any state changes - * pushed onto the component. - */ -class FakeComponent

{ - constructor( - public state: LiftWrapperState, - public props: LiftWrapperProps

- ) {} - - setState(state: (LiftWrapperState | ((state: LiftWrapperState) => LiftWrapperState))) { - const newState = typeof state === 'function' ? state(this.state) : state - - if ('subscription' in newState) - this.state.subscription = newState.subscription - if ('renderCache' in newState) - this.state.renderCache = newState.renderCache - } -} - // could make sense to make this configurable const handleError = (e: any) => { throw e @@ -352,27 +368,21 @@ function warnEmptyObservable(componentName: string | undefined) { * * @template P component props */ -interface LiftWrapperType

{ - state: LiftWrapperState - props: LiftWrapperProps

- setState(state: (LiftWrapperState | ((state: LiftWrapperState) => LiftWrapperState))): void -} - class RenderOne

implements Subscription { - private _liftedComponent: LiftWrapperType

+ private _liftedComponent: LiftWrapper.Type

private _innerSubscription: RxSubscription | null = null private _receivedValue = false constructor( - liftedComponent: LiftWrapperType

, - newProps: LiftWrapperProps

+ liftedComponent: LiftWrapper.Type

, + newProps: LiftWrapper.Props

) { - const state: LiftWrapperState = { + const state: LiftWrapper.State = { subscription: this, renderCache: liftedComponent.state && liftedComponent.state.renderCache } - this._liftedComponent = new FakeComponent

(state, newProps) + this._liftedComponent = new LiftWrapper.FakeRenderer

(state, newProps) walkObservables( newProps.props, @@ -414,7 +424,7 @@ class RenderOne

implements Subscription { private _handleCompleted() { this._innerSubscription = null - this._liftedComponent.setState(LiftWrapperII._endState) + this._liftedComponent.setState(LiftWrapper.endState) } } @@ -424,21 +434,21 @@ class RenderOne

implements Subscription { * @template P component props */ class RenderMany

implements Subscription { - private _liftedComponent: LiftWrapperType

+ private _liftedComponent: LiftWrapper.Type

private _values: any[] private _innerSubscriptions: (RxSubscription | null)[] constructor( - liftedComponent: LiftWrapperType

, - newProps: LiftWrapperProps

, + liftedComponent: LiftWrapper.Type

, + newProps: LiftWrapper.Props

, N: number ) { - const state: LiftWrapperState = { + const state: LiftWrapper.State = { subscription: this, renderCache: liftedComponent.state && liftedComponent.state.renderCache } - this._liftedComponent = new FakeComponent(state, newProps) + this._liftedComponent = new LiftWrapper.FakeRenderer(state, newProps) this._innerSubscriptions = [] this._values = Array(N) @@ -521,7 +531,7 @@ class RenderMany

implements Subscription { if (this._innerSubscriptions[i]) return - this._liftedComponent.setState(LiftWrapperII._endState) + this._liftedComponent.setState(LiftWrapper.endState) } } From 4c0a12550ea4ddd4f0ebf39a71808d6616986bab Mon Sep 17 00:00:00 2001 From: Ostap Chervak Date: Thu, 8 Apr 2021 16:51:08 +0300 Subject: [PATCH 3/5] More refactor. Add strict mode to examples & tests --- packages/examples/all/src/index.tsx | 2 +- packages/examples/todomvc/src/app.tsx | 2 +- packages/focal/src/react/react.ts | 109 +++++++++----------------- packages/focal/test/react.test.tsx | 5 +- 4 files changed, 45 insertions(+), 73 deletions(-) diff --git a/packages/examples/all/src/index.tsx b/packages/examples/all/src/index.tsx index badf8a6..ebefbca 100644 --- a/packages/examples/all/src/index.tsx +++ b/packages/examples/all/src/index.tsx @@ -23,7 +23,7 @@ function startApp(C: typeof App.AppComponent) { if (targetEl == null) throw new Error('React app target element not found. Wrong HTML file?') - ReactDOM.render(, targetEl) + ReactDOM.render(, targetEl) } if (module.hot) { diff --git a/packages/examples/todomvc/src/app.tsx b/packages/examples/todomvc/src/app.tsx index 8bdd480..26bfd13 100644 --- a/packages/examples/todomvc/src/app.tsx +++ b/packages/examples/todomvc/src/app.tsx @@ -209,7 +209,7 @@ export class App { start() { ReactDOM.render( - , + , this._targetElement ) } diff --git a/packages/focal/src/react/react.ts b/packages/focal/src/react/react.ts index 5dc78e7..9ffca3f 100644 --- a/packages/focal/src/react/react.ts +++ b/packages/focal/src/react/react.ts @@ -29,19 +29,18 @@ export namespace LiftWrapper { props: Lifted } - type RenderCache = React.DOMElement | null - type WrapperSubscription = Subscription | null + export type RenderCache = React.DOMElement | null + export type WrapperSubscription = Subscription | null export interface State { renderCache?: RenderCache subscription?: WrapperSubscription } - export interface Type

{ - state: State - props: Props

- setState(state: (State | ((state: State) => State))): void + export interface SetState { + (state: (State | ((state: State) => State))): void } + function useForceUpdate(): () => void { return React.useReducer(() => ({}), {})[1] as () => void } @@ -64,11 +63,12 @@ export namespace LiftWrapper { // eslint-disable-next-line @typescript-eslint/no-use-before-define walkObservables(props, () => n += 1) - const state = { subscription: subscription.current, renderCache: renderCache.current } - + // variable to track sync emit from observable in Render{One, Many}. + // prevent waste render during initial render & re-subscribe let inSync = false - const setState = (s: (State | ((s: State) => State))) => { + const setState: SetState = (s: (State | ((s: State) => State))) => { + const state = { subscription: subscription.current, renderCache: renderCache.current } const newState = typeof s === 'function' ? s(state) : s if (newState.subscription) { @@ -101,36 +101,16 @@ export namespace LiftWrapper { // Could this be replaced by a regular closure? Perhaps using // a class is an optimization? case 1: - new RenderOne({ props: newProps, state, setState }, newProps) // eslint-disable-line + new RenderOne(renderCache, newProps, setState) // eslint-disable-line break default: - new RenderMany({ props: newProps, state, setState }, newProps, n) // eslint-disable-line + new RenderMany(renderCache, newProps, setState, n) // eslint-disable-line break } inSync = true } -/** - * A dummy React component mock that is used to preserve any state changes - * pushed onto the component. - */ -export class FakeRenderer

implements LiftWrapper.Type

{ - constructor( - public state: LiftWrapper.State, - public props: LiftWrapper.Props

- ) {} - - setState(state: (LiftWrapper.State | ((state: LiftWrapper.State) => LiftWrapper.State))) { - const newState = typeof state === 'function' ? state(this.state) : state - - if ('subscription' in newState) - this.state.subscription = newState.subscription - if ('renderCache' in newState) - this.state.renderCache = newState.renderCache - } -} - export const initState = { renderCache: null, subscription: null @@ -369,28 +349,22 @@ function warnEmptyObservable(componentName: string | undefined) { * @template P component props */ class RenderOne

implements Subscription { - private _liftedComponent: LiftWrapper.Type

private _innerSubscription: RxSubscription | null = null private _receivedValue = false constructor( - liftedComponent: LiftWrapper.Type

, - newProps: LiftWrapper.Props

+ renderCache: React.MutableRefObject, + private newProps: LiftWrapper.Props

, + private setState: LiftWrapper.SetState ) { - const state: LiftWrapper.State = { - subscription: this, - renderCache: liftedComponent.state && liftedComponent.state.renderCache - } - - this._liftedComponent = new LiftWrapper.FakeRenderer

(state, newProps) - walkObservables( newProps.props, observable => { this._innerSubscription = observable.subscribe( - (v: any) => this._handleValue(v), + this._handleValue, handleError, - () => this._handleCompleted()) + this._handleCompleted + ) // observable has completed and unsubscribed by itself if (this._innerSubscription && this._innerSubscription.closed) @@ -398,10 +372,12 @@ class RenderOne

implements Subscription { }) if (DEV_ENV && !this._receivedValue) - warnEmptyObservable(getReactComponentName(this._liftedComponent.props.component)) + warnEmptyObservable(getReactComponentName(newProps.component)) - this._liftedComponent = liftedComponent - liftedComponent.setState(state) + setState({ + subscription: this, + renderCache: renderCache.current + }) } unsubscribe() { @@ -409,22 +385,21 @@ class RenderOne

implements Subscription { this._innerSubscription.unsubscribe() } - private _handleValue(value: any) { + private _handleValue = (value: any) => { // only required for empty observable check if (DEV_ENV) this._receivedValue = true - const liftedComponent = this._liftedComponent - const { component, props } = liftedComponent.props + const { component, props } = this.newProps const renderCache = render(component, props, [value]) - liftedComponent.setState(state => + this.setState(state => !structEq(state.renderCache, renderCache) ? { renderCache } : {} ) } - private _handleCompleted() { + private _handleCompleted = () => { this._innerSubscription = null - this._liftedComponent.setState(LiftWrapper.endState) + this.setState(LiftWrapper.endState) } } @@ -434,22 +409,15 @@ class RenderOne

implements Subscription { * @template P component props */ class RenderMany

implements Subscription { - private _liftedComponent: LiftWrapper.Type

private _values: any[] private _innerSubscriptions: (RxSubscription | null)[] constructor( - liftedComponent: LiftWrapper.Type

, - newProps: LiftWrapper.Props

, + renderCache: React.MutableRefObject, + private newProps: LiftWrapper.Props

, + private setState: LiftWrapper.SetState, N: number ) { - const state: LiftWrapper.State = { - subscription: this, - renderCache: liftedComponent.state && liftedComponent.state.renderCache - } - - this._liftedComponent = new LiftWrapper.FakeRenderer(state, newProps) - this._innerSubscriptions = [] this._values = Array(N) @@ -483,17 +451,19 @@ class RenderMany

implements Subscription { if (DEV_ENV) for (let i = this._values.length - 1; 0 <= i; --i) if (this._values[i] === this) { - warnEmptyObservable(getReactComponentName(liftedComponent.props.component)) + warnEmptyObservable(getReactComponentName(newProps.component)) break } - this._liftedComponent = liftedComponent - liftedComponent.setState(state) + this.setState({ + subscription: this, + renderCache: renderCache.current + }) } unsubscribe() { let i = -1 - walkObservables(this._liftedComponent.props.props, _ => { + walkObservables(this.newProps.props, _ => { const unsubscriber = this._innerSubscriptions[++i] if (unsubscriber) unsubscriber.unsubscribe() @@ -509,11 +479,10 @@ class RenderMany

implements Subscription { if (this._values[i] === this) return - const liftedComponent = this._liftedComponent - const { component, props } = liftedComponent.props + const { component, props } = this.newProps const renderCache = render(component, props, this._values) - liftedComponent.setState(state => + this.setState(state => !structEq(state.renderCache, renderCache) ? { renderCache } : {} ) } @@ -531,7 +500,7 @@ class RenderMany

implements Subscription { if (this._innerSubscriptions[i]) return - this._liftedComponent.setState(LiftWrapper.endState) + this.setState(LiftWrapper.endState) } } diff --git a/packages/focal/test/react.test.tsx b/packages/focal/test/react.test.tsx index 2a1bb3e..c2fbaaf 100644 --- a/packages/focal/test/react.test.tsx +++ b/packages/focal/test/react.test.tsx @@ -20,7 +20,10 @@ class Comp extends React.Component<{ test: string }, {}> { } function testRender(actual: JSX.Element | null, expected: string, desc: string) { - it(desc, () => expect(actual && ReactDOM.renderToStaticMarkup(actual)).toEqual(expected)) + it(desc, () => expect( + actual && ReactDOM.renderToStaticMarkup({actual}) + ).toEqual(expected) + ) } describe('react', () => { From 48ba1425f0a8b8d317ba09703ff3348ff32bb7ba Mon Sep 17 00:00:00 2001 From: Ostap Chervak Date: Thu, 8 Apr 2021 21:28:21 +0300 Subject: [PATCH 4/5] Verify concurrent mode support. Moar refactor --- packages/examples/all/src/app.tsx | 2 +- packages/examples/all/src/player/index.tsx | 3 +- packages/focal/src/react/react.ts | 97 ++++++++++------------ packages/test/src/setstate-bug/index.tsx | 5 +- 4 files changed, 49 insertions(+), 58 deletions(-) diff --git a/packages/examples/all/src/app.tsx b/packages/examples/all/src/app.tsx index d57474d..e367c74 100644 --- a/packages/examples/all/src/app.tsx +++ b/packages/examples/all/src/app.tsx @@ -2,7 +2,7 @@ import * as React from 'react' import { Atom, Lens } from '@grammarly/focal' interface ExampleComponent { - Component: React.StatelessComponent<{ state: Atom }> + Component: React.FunctionComponent<{ state: Atom }> defaultState: S } diff --git a/packages/examples/all/src/player/index.tsx b/packages/examples/all/src/player/index.tsx index 3a7eb37..98938b8 100644 --- a/packages/examples/all/src/player/index.tsx +++ b/packages/examples/all/src/player/index.tsx @@ -52,7 +52,8 @@ const TimeLine = ({ class App extends React.Component<{ state: Atom }, {}> { audioModel: AudioModel - componentWillMount() { + // eslint-disable-next-line camelcase + UNSAFE_componentWillMount() { this.audioModel = new AudioModel(this.props.state, audioSrc) } diff --git a/packages/focal/src/react/react.ts b/packages/focal/src/react/react.ts index 9ffca3f..f69317b 100644 --- a/packages/focal/src/react/react.ts +++ b/packages/focal/src/react/react.ts @@ -32,13 +32,12 @@ export namespace LiftWrapper { export type RenderCache = React.DOMElement | null export type WrapperSubscription = Subscription | null - export interface State { - renderCache?: RenderCache - subscription?: WrapperSubscription + export interface SetRenderCache { + (cache: RenderCache): void } - export interface SetState { - (state: (State | ((state: State) => State))): void + export interface SetSubscription { + (sub: WrapperSubscription): void } function useForceUpdate(): () => void { @@ -67,32 +66,25 @@ export namespace LiftWrapper { // prevent waste render during initial render & re-subscribe let inSync = false - const setState: SetState = (s: (State | ((s: State) => State))) => { - const state = { subscription: subscription.current, renderCache: renderCache.current } - const newState = typeof s === 'function' ? s(state) : s - - if (newState.subscription) { - subscription.current = newState.subscription + const setCache: SetRenderCache = cache => { + if (renderCache.current === cache) { + return } - if (newState.renderCache) { - if (renderCache.current === newState.renderCache) { - return - } + renderCache.current = cache - renderCache.current = newState.renderCache - - if (inSync) { - forceUpdate() - } + if (inSync) { + forceUpdate() } } + const setSubscription: SetSubscription = sub => subscription.current = sub + switch (n) { case 0: - subscription.current = null + setSubscription(null) // eslint-disable-next-line @typescript-eslint/no-use-before-define - renderCache.current = render(component, props) + setCache(render(component, props)) break // @NOTE original Calmm code below @@ -101,28 +93,29 @@ export namespace LiftWrapper { // Could this be replaced by a regular closure? Perhaps using // a class is an optimization? case 1: - new RenderOne(renderCache, newProps, setState) // eslint-disable-line + new RenderOne(renderCache, newProps, setCache, setSubscription) // eslint-disable-line break default: - new RenderMany(renderCache, newProps, setState, n) // eslint-disable-line + new RenderMany(renderCache, newProps, setCache, setSubscription, n) // eslint-disable-line break } inSync = true } - export const initState = { - renderCache: null, - subscription: null - } + export const Renderer = (props: Props) => { + const _subscription = React.useRef(null) + const _renderCache = React.useRef(null) - export const endState = { - subscription: null - } + // concurrent mode support + // see https://codesandbox.io/s/x2p46v02z4?from-embed=&file=/src/BadCounter.jsx + const subscription = { current: _subscription.current } + const renderCache = { current: _renderCache.current } - export const Renderer = (props: Props) => { - const subscription = React.useRef(initState.subscription) - const renderCache = React.useRef(initState.renderCache) + React.useEffect(() => { + _subscription.current = subscription.current + _renderCache.current = renderCache.current + }) React.useEffect(() => () => unsubscribe(subscription), []) @@ -353,9 +346,10 @@ class RenderOne

implements Subscription { private _receivedValue = false constructor( - renderCache: React.MutableRefObject, + private renderCache: React.MutableRefObject, private newProps: LiftWrapper.Props

, - private setState: LiftWrapper.SetState + private setRenderCache: LiftWrapper.SetRenderCache, + private setSubscription: LiftWrapper.SetSubscription ) { walkObservables( newProps.props, @@ -374,10 +368,7 @@ class RenderOne

implements Subscription { if (DEV_ENV && !this._receivedValue) warnEmptyObservable(getReactComponentName(newProps.component)) - setState({ - subscription: this, - renderCache: renderCache.current - }) + this.setSubscription(this) } unsubscribe() { @@ -392,14 +383,14 @@ class RenderOne

implements Subscription { const { component, props } = this.newProps const renderCache = render(component, props, [value]) - this.setState(state => - !structEq(state.renderCache, renderCache) ? { renderCache } : {} - ) + if (!structEq(this.renderCache.current, renderCache)) { + this.setRenderCache(renderCache) + } } private _handleCompleted = () => { this._innerSubscription = null - this.setState(LiftWrapper.endState) + this.setSubscription(null) } } @@ -413,9 +404,10 @@ class RenderMany

implements Subscription { private _innerSubscriptions: (RxSubscription | null)[] constructor( - renderCache: React.MutableRefObject, + private renderCache: React.MutableRefObject, private newProps: LiftWrapper.Props

, - private setState: LiftWrapper.SetState, + private setRenderCache: LiftWrapper.SetRenderCache, + private setSubscription: LiftWrapper.SetSubscription, N: number ) { this._innerSubscriptions = [] @@ -455,10 +447,7 @@ class RenderMany

implements Subscription { break } - this.setState({ - subscription: this, - renderCache: renderCache.current - }) + this.setSubscription(this) } unsubscribe() { @@ -482,9 +471,9 @@ class RenderMany

implements Subscription { const { component, props } = this.newProps const renderCache = render(component, props, this._values) - this.setState(state => - !structEq(state.renderCache, renderCache) ? { renderCache } : {} - ) + if (!structEq(this.renderCache.current, renderCache)) { + this.setRenderCache(renderCache) + } } private _handleCompleted(idx: number) { @@ -500,7 +489,7 @@ class RenderMany

implements Subscription { if (this._innerSubscriptions[i]) return - this.setState(LiftWrapper.endState) + this.setSubscription(null) } } diff --git a/packages/test/src/setstate-bug/index.tsx b/packages/test/src/setstate-bug/index.tsx index b68c5ef..9315e38 100644 --- a/packages/test/src/setstate-bug/index.tsx +++ b/packages/test/src/setstate-bug/index.tsx @@ -5,7 +5,8 @@ import { Observable, Subject } from 'rxjs' class Test extends React.Component<{ trigger: Observable }> { list = Atom.create(['a', 'b', 'c']) - componentWillMount() { + // eslint-disable-next-line camelcase + UNSAFE_componentWillMount() { this.props.trigger.subscribe(_ => { this.list.set(['a', 'b']) this.list.set(['a', 'b', 'c']) @@ -13,7 +14,7 @@ class Test extends React.Component<{ trigger: Observable }> { } render() { - return ( + return ( <>

Focal

From 2049eb9a32bdd2481e0b905c748fb4335152aae0 Mon Sep 17 00:00:00 2001 From: Ostap Chervak Date: Thu, 27 May 2021 17:16:29 +0300 Subject: [PATCH 5/5] Re-write solution to classes, because of perf issues with hooks --- .gitignore | 1 + packages/focal/src/react/react.ts | 158 ++++++++++++++---------------- 2 files changed, 73 insertions(+), 86 deletions(-) diff --git a/.gitignore b/.gitignore index 19a5591..05ee6f3 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ +.DS_Store logs *.log npm-debug.log* diff --git a/packages/focal/src/react/react.ts b/packages/focal/src/react/react.ts index f69317b..b6fecfc 100644 --- a/packages/focal/src/react/react.ts +++ b/packages/focal/src/react/react.ts @@ -32,100 +32,90 @@ export namespace LiftWrapper { export type RenderCache = React.DOMElement | null export type WrapperSubscription = Subscription | null - export interface SetRenderCache { - (cache: RenderCache): void - } + export class Renderer extends React.PureComponent> { + static displayName = 'LiftWrapper' - export interface SetSubscription { - (sub: WrapperSubscription): void - } + subscription: WrapperSubscription = null + renderCache: RenderCache = null - function useForceUpdate(): () => void { - return React.useReducer(() => ({}), {})[1] as () => void - } + // variable to track sync emit from observable in Render{One, Many}. + // prevent waste render during initial render & re-subscribe + isSubscribed = false - const unsubscribe = (subscription: React.MutableRefObject) => { - if (subscription.current) { - subscription.current.unsubscribe() + // eslint-disable-next-line camelcase + UNSAFE_componentWillMount() { + this._subscribe(this.props) } - } - const useSubscribe = ( - newProps: Props, - subscription: React.MutableRefObject, - renderCache: React.MutableRefObject - ) => { - const forceUpdate = useForceUpdate() - const { props, component } = newProps - - let n = 0 - // eslint-disable-next-line @typescript-eslint/no-use-before-define - walkObservables(props, () => n += 1) + // eslint-disable-next-line camelcase + UNSAFE_componentWillReceiveProps(nextProps: Props) { + this._unsubscribe() + this._subscribe(nextProps) + } - // variable to track sync emit from observable in Render{One, Many}. - // prevent waste render during initial render & re-subscribe - let inSync = false + componentWillUnmount() { + this._unsubscribe() + this.setSubscription(null) + } - const setCache: SetRenderCache = cache => { - if (renderCache.current === cache) { + setRenderCache(cache: RenderCache): void { + if (this.renderCache === cache) { return } - renderCache.current = cache + this.renderCache = cache - if (inSync) { - forceUpdate() + if (this.isSubscribed) { + this.forceUpdate() } } - const setSubscription: SetSubscription = sub => subscription.current = sub - - switch (n) { - case 0: - setSubscription(null) - // eslint-disable-next-line @typescript-eslint/no-use-before-define - setCache(render(component, props)) - break - - // @NOTE original Calmm code below - // The created object is never used and it looks like that - // the useful work is done in the constructor. - // Could this be replaced by a regular closure? Perhaps using - // a class is an optimization? - case 1: - new RenderOne(renderCache, newProps, setCache, setSubscription) // eslint-disable-line - break - default: - new RenderMany(renderCache, newProps, setCache, setSubscription, n) // eslint-disable-line - break + setSubscription(sub: WrapperSubscription): void { + this.subscription = sub } - inSync = true - } + render() { + return this.renderCache || null + } - export const Renderer = (props: Props) => { - const _subscription = React.useRef(null) - const _renderCache = React.useRef(null) + private _unsubscribe() { + if (this.subscription) { + this.subscription.unsubscribe() + } + } - // concurrent mode support - // see https://codesandbox.io/s/x2p46v02z4?from-embed=&file=/src/BadCounter.jsx - const subscription = { current: _subscription.current } - const renderCache = { current: _renderCache.current } + private _subscribe(newProps: Props) { + const { props, component } = newProps - React.useEffect(() => { - _subscription.current = subscription.current - _renderCache.current = renderCache.current - }) + let n = 0 + // eslint-disable-next-line @typescript-eslint/no-use-before-define + walkObservables(props, () => n += 1) - React.useEffect(() => () => unsubscribe(subscription), []) + this.isSubscribed = false - unsubscribe(subscription) - useSubscribe(props, subscription, renderCache) + switch (n) { + case 0: + this.setSubscription(null) + // eslint-disable-next-line @typescript-eslint/no-use-before-define + this.setRenderCache(render(component, props)) + break - return renderCache.current || null - } + // @NOTE original Calmm code below + // The created object is never used and it looks like that + // the useful work is done in the constructor. + // Could this be replaced by a regular closure? Perhaps using + // a class is an optimization? + case 1: + new RenderOne(this, newProps) // eslint-disable-line + break + default: + new RenderMany(this, newProps, n) // eslint-disable-line + break + } - Renderer.displayName = 'LiftWrapper' + this.isSubscribed = true + } + } } // here we only say TProps, but a lifted component @@ -346,10 +336,8 @@ class RenderOne

implements Subscription { private _receivedValue = false constructor( - private renderCache: React.MutableRefObject, - private newProps: LiftWrapper.Props

, - private setRenderCache: LiftWrapper.SetRenderCache, - private setSubscription: LiftWrapper.SetSubscription + private renderer: LiftWrapper.Renderer

, + private newProps: LiftWrapper.Props

) { walkObservables( newProps.props, @@ -368,7 +356,7 @@ class RenderOne

implements Subscription { if (DEV_ENV && !this._receivedValue) warnEmptyObservable(getReactComponentName(newProps.component)) - this.setSubscription(this) + this.renderer.setSubscription(this) } unsubscribe() { @@ -383,14 +371,14 @@ class RenderOne

implements Subscription { const { component, props } = this.newProps const renderCache = render(component, props, [value]) - if (!structEq(this.renderCache.current, renderCache)) { - this.setRenderCache(renderCache) + if (!structEq(this.renderer.renderCache, renderCache)) { + this.renderer.setRenderCache(renderCache) } } private _handleCompleted = () => { this._innerSubscription = null - this.setSubscription(null) + this.renderer.setSubscription(null) } } @@ -404,10 +392,8 @@ class RenderMany

implements Subscription { private _innerSubscriptions: (RxSubscription | null)[] constructor( - private renderCache: React.MutableRefObject, + private renderer: LiftWrapper.Renderer

, private newProps: LiftWrapper.Props

, - private setRenderCache: LiftWrapper.SetRenderCache, - private setSubscription: LiftWrapper.SetSubscription, N: number ) { this._innerSubscriptions = [] @@ -447,7 +433,7 @@ class RenderMany

implements Subscription { break } - this.setSubscription(this) + this.renderer.setSubscription(this) } unsubscribe() { @@ -471,8 +457,8 @@ class RenderMany

implements Subscription { const { component, props } = this.newProps const renderCache = render(component, props, this._values) - if (!structEq(this.renderCache.current, renderCache)) { - this.setRenderCache(renderCache) + if (!structEq(this.renderer.renderCache, renderCache)) { + this.renderer.setRenderCache(renderCache) } } @@ -489,7 +475,7 @@ class RenderMany

implements Subscription { if (this._innerSubscriptions[i]) return - this.setSubscription(null) + this.renderer.setSubscription(null) } }