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/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/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/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/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/intrinsic.ts b/packages/focal/src/react/intrinsic.ts index 3722c99..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, LiftWrapper } 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>>( - LiftWrapper, + 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(LiftWrapper, { 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 adce812..b6fecfc 100644 --- a/packages/focal/src/react/react.ts +++ b/packages/focal/src/react/react.ts @@ -15,111 +15,113 @@ 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 -} + export type RenderCache = React.DOMElement | null + export type WrapperSubscription = 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 + export class Renderer extends React.PureComponent> { + static displayName = 'LiftWrapper' - static _initState: LiftWrapperState = { - renderCache: null, - subscription: null - } + subscription: WrapperSubscription = null + renderCache: RenderCache = null - static _endState: LiftWrapperState = { - subscription: null - } + // variable to track sync emit from observable in Render{One, Many}. + // prevent waste render during initial render & re-subscribe + isSubscribed = false - render() { - return this.state.renderCache || null - } + // eslint-disable-next-line camelcase + UNSAFE_componentWillMount() { + this._subscribe(this.props) + } - private _subscribe(newProps: LiftWrapperProps) { - const { props, component } = newProps + // eslint-disable-next-line camelcase + UNSAFE_componentWillReceiveProps(nextProps: Props) { + this._unsubscribe() + this._subscribe(nextProps) + } - let n = 0 - // eslint-disable-next-line @typescript-eslint/no-use-before-define - walkObservables(props, () => n += 1) + componentWillUnmount() { + this._unsubscribe() + this.setSubscription(null) + } - switch (n) { - case 0: - this.setState({ - subscription: null, - // eslint-disable-next-line @typescript-eslint/no-use-before-define - renderCache: 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(this, newProps) // eslint-disable-line - break - default: - new RenderMany(this, newProps, n) // eslint-disable-line - break + setRenderCache(cache: RenderCache): void { + if (this.renderCache === cache) { + return + } + + this.renderCache = cache + + if (this.isSubscribed) { + this.forceUpdate() + } } - } - private _unsubscribe() { - const subscription = this.state ? this.state.subscription : null - if (subscription) - subscription.unsubscribe() - } + setSubscription(sub: WrapperSubscription): void { + this.subscription = sub + } - // eslint-disable-next-line camelcase - UNSAFE_componentWillReceiveProps(newProps: LiftWrapperProps) { - this._unsubscribe() - this._subscribe(newProps) - } + render() { + return this.renderCache || null + } - // eslint-disable-next-line camelcase - UNSAFE_componentWillMount() { - this._unsubscribe() - this._subscribe(this.props) - } + private _unsubscribe() { + if (this.subscription) { + this.subscription.unsubscribe() + } + } - componentWillUnmount() { - this._unsubscribe() - this.setState(LiftWrapper._initState) - } + private _subscribe(newProps: Props) { + const { props, component } = newProps - shouldComponentUpdate( - _newProps: Readonly>, - newState: Readonly, - _newContext: any - ) { - return newState.renderCache !== this.state.renderCache + let n = 0 + // eslint-disable-next-line @typescript-eslint/no-use-before-define + walkObservables(props, () => n += 1) + + this.isSubscribed = false + + switch (n) { + case 0: + this.setSubscription(null) + // eslint-disable-next-line @typescript-eslint/no-use-before-define + this.setRenderCache(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(this, newProps) // eslint-disable-line + break + default: + new RenderMany(this, newProps, n) // eslint-disable-line + break + } + + this.isSubscribed = true + } } } // 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 } @@ -154,8 +156,8 @@ export function lift( component: React.ComponentClass | React.StatelessComponent ) { return (props: LiftedComponentProps) => - React.createElement>( - LiftWrapper, + React.createElement>( + LiftWrapper.Renderer, { component: component, props: props } ) } @@ -175,7 +177,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) { @@ -220,7 +222,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? @@ -309,26 +311,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 @@ -350,29 +332,21 @@ function warnEmptyObservable(componentName: string | undefined) { * @template P component props */ class RenderOne

implements Subscription { - private _liftedComponent: LiftWrapper

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

, - newProps: LiftWrapperProps

+ private renderer: LiftWrapper.Renderer

, + private newProps: LiftWrapper.Props

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

(state, newProps) as LiftWrapper

- 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) @@ -380,10 +354,9 @@ 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) + this.renderer.setSubscription(this) } unsubscribe() { @@ -391,22 +364,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 => - !structEq(state.renderCache, renderCache) ? { renderCache } : {} - ) + if (!structEq(this.renderer.renderCache, renderCache)) { + this.renderer.setRenderCache(renderCache) + } } - private _handleCompleted() { + private _handleCompleted = () => { this._innerSubscription = null - this._liftedComponent.setState(LiftWrapper._endState) + this.renderer.setSubscription(null) } } @@ -416,23 +388,14 @@ class RenderOne

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

implements Subscription { - private _liftedComponent: LiftWrapper

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

, - newProps: LiftWrapperProps

, + private renderer: LiftWrapper.Renderer

, + private newProps: LiftWrapper.Props

, N: number ) { - const state: LiftWrapperState = { - subscription: this, - renderCache: liftedComponent.state && liftedComponent.state.renderCache - } - - this._liftedComponent = - new FakeComponent(state, newProps) as LiftWrapper

- this._innerSubscriptions = [] this._values = Array(N) @@ -466,17 +429,16 @@ 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.renderer.setSubscription(this) } unsubscribe() { let i = -1 - walkObservables(this._liftedComponent.props.props, _ => { + walkObservables(this.newProps.props, _ => { const unsubscriber = this._innerSubscriptions[++i] if (unsubscriber) unsubscriber.unsubscribe() @@ -492,13 +454,12 @@ 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 => - !structEq(state.renderCache, renderCache) ? { renderCache } : {} - ) + if (!structEq(this.renderer.renderCache, renderCache)) { + this.renderer.setRenderCache(renderCache) + } } private _handleCompleted(idx: number) { @@ -514,7 +475,7 @@ class RenderMany

implements Subscription { if (this._innerSubscriptions[i]) return - this._liftedComponent.setState(LiftWrapper._endState) + this.renderer.setSubscription(null) } } 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', () => { 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