From 5ac6e0af12552d87ac9799d411bc899507f96d63 Mon Sep 17 00:00:00 2001 From: Pablo Palacios Date: Tue, 1 Jun 2021 18:20:54 +0200 Subject: [PATCH] feat: Improve ref support on connectToStores (and remove refs from other components) (#696) --- package.json | 2 +- packages/fluxible-addons-react/UPGRADE.md | 91 +++++-- .../src/connectToStores.js | 75 +++--- .../src/provideContext.js | 13 +- .../tests/unit/lib/batchedUpdatePlugin.js | 157 ++++++------ .../tests/unit/lib/connectToStores.js | 228 ++++++++++-------- .../tests/unit/lib/provideContext.js | 39 --- .../fluxible-router/src/lib/handleHistory.js | 6 +- .../fluxible-router/src/lib/handleRoute.js | 8 +- .../tests/unit/lib/handleHistory-test.js | 31 --- .../tests/unit/lib/handleRoute-test.js | 87 ++----- 11 files changed, 365 insertions(+), 372 deletions(-) diff --git a/package.json b/package.json index 7e1dc5be..a0fff5c1 100644 --- a/package.json +++ b/package.json @@ -16,7 +16,6 @@ "lint": "eslint .", "test": "lerna run lint --since --stream && lerna run test --since --stream" }, - "dependencies": {}, "devDependencies": { "@babel/cli": "^7.11.6", "@babel/core": "^7.11.6", @@ -43,6 +42,7 @@ "pre-commit": "^1.0.7", "react": "^16.0.0", "react-dom": "^16.0.0", + "react-test-renderer": "^16.0.0", "shelljs": "^0.8.0", "sinon": "^9.0.1", "yargs": "^16.0.0" diff --git a/packages/fluxible-addons-react/UPGRADE.md b/packages/fluxible-addons-react/UPGRADE.md index 6b7239bd..a71c6516 100644 --- a/packages/fluxible-addons-react/UPGRADE.md +++ b/packages/fluxible-addons-react/UPGRADE.md @@ -51,13 +51,18 @@ If you were using `provideContext` to provide other context data not related to fluxible itself, you will need to provide your own solution to achieve the same result as before. +#### Ref support removed + +This hoc does not include `wrappedComponent` ref anymore. + ### connectToStores -`connectToStores(Component, stores, getStateFromStores, customContextTypes)` -> `connectToStores(Component, stores, getStateFromStores)` +`connectToStores(Component, stores, getStateFromStores, customContextTypes)` -> `connectToStores(Component, stores, getStateFromStores, options)` Since the new React API doesn't rely on PropTypes anymore, there is no -need to specify customContextTypes to extract plugins context from the -fluxible context since all plugins will be available. +need to specify the `customContextTypes` param to extract plugins +context from the fluxible context. All plugins component context are +available. **Before:** @@ -67,30 +72,80 @@ const customContextTypes = { pluginFoo: PropTypes.object }; -connectToStores(Component, customContextTypes) +connectToStores(Component, stores, getStateFromStores, customContextTypes) ``` **After:** ```javascript -connectToStores(Component) +connectToStores(Component, stores, getStateFromStores) ``` -If you were relying in other contextTypes that were not included in -`provideContext` (non fluxible plugins), you will need to find another -way to have it available in `getStateFromStores`. Since the second -argument of `getStateFromStores` is the props passed to the wrapped -component, you can create your own high order component that passes -the required context as props to your connected component: +#### Ref support improved + +`connectToStores` now returns a `React.forwardRef` component instead +of internally attaching the `wrappedComponent` ref to the wrapped +component. This means that you can now pass a ref to the connected +component that it will be forwarded to the wrapped component. + +However, in order to have your prop forwarded, you need to explicitly +tell it to `connectToStores` by setting `forwardRef` to `true` in the +`options` param. + +**Before** ```javascript -const customContextTypes = { - foo: PropTypes.string, - bar: PropTypes.object -}; +// Only class components would get access to a ref +class CustomInput extends React.Component { + constructor() { + this.ref = React.createRef(); + } + + render() { + return + } +} -export default injectContext(customContextTypes, connectToStores(Component)); +const ConnectedInput = connectToStores(CustomInput, stores, getStateFromStores); + +class App extends React.Component { + constructor() { + this.ref = React.createRef(); + } + + componentDidMount() { + this.ref.current.wrappedComponent.ref.current.focus() + } + + render() { + return + } +} ``` -You can read more about how to create your own high order components -at React [official documentation](https://reactjs.org/docs/higher-order-components.html). +**After** +```javascript +// Besides classes, it's now possible to forward refs to functional components. +const CustomInput = React.forwardRef((props, ref) => ); + +const ConnectedInput = connectToStores( + CustomInput, + stores, + getStateFromStores, + { forwardRef: true } +); + +class App extends React.Component { + constructor() { + this.ref = React.createRef(); + } + + componentDidMount() { + this.ref.current.focus() + } + + render() { + return + } +} +``` diff --git a/packages/fluxible-addons-react/src/connectToStores.js b/packages/fluxible-addons-react/src/connectToStores.js index f68a7a03..9aaad9bb 100644 --- a/packages/fluxible-addons-react/src/connectToStores.js +++ b/packages/fluxible-addons-react/src/connectToStores.js @@ -2,32 +2,36 @@ * Copyright 2015, Yahoo Inc. * Copyrights licensed under the New BSD License. See the accompanying LICENSE file for terms. */ -import { Component as ReactComponent, createRef, createElement } from 'react'; +import { Component as ReactComponent, createElement, forwardRef } from 'react'; import hoistNonReactStatics from 'hoist-non-react-statics'; import { FluxibleContext } from './FluxibleContext'; /** - * Registers change listeners and retrieves state from stores using the `getStateFromStores` - * method. Concept provided by Dan Abramov via - * https://medium.com/@dan_abramov/mixins-are-dead-long-live-higher-order-components-94a0d2f9e750 + * @callback getStateFromStores + * @param {FluxibleContext} context - Fluxible component context. + * @param {Object} ownProps - The props that the component received. + * @returns {Object} props - The props that should be passed to the component. + */ + +/** + * HOC that registers change listeners and retrieves state from stores + * using the `getStateFromStores` method. * * Example: - * connectToStores(Component, [FooStore], { - * FooStore: function (store, props) { - * return { - * foo: store.getFoo() - * } - * } + * connectToStores(Component, [FooStore], (context, props) => ({ + * foo: context.getStore(FooStore).getFoo(), + * onClick: () => context.executeAction(fooAction, props) * }) * - * @method connectToStores - * @param {React.Component} [Component] component to pass state as props to. - * @param {array} stores List of stores to listen for changes - * @param {function} getStateFromStores function that receives all stores and should return - * the full state object. Receives `stores` hash and component `props` as arguments - * @returns {React.Component} or {Function} if using decorator pattern + * @function connectToStores + * @param {React.Component} Component - The component to pass state as props to. + * @param {array} stores - List of stores to listen for changes. + * @param {getStateFromStores} getStateFromStores - The main function that must map the context into props. + * @param {Object} [options] - options to tweak the HOC. + * @param {boolean} options.forwardRef - If true, forwards a ref to the wrapped component. + * @returns {React.Component} ConnectedComponent - A component connected to the stores. */ -function connectToStores(Component, stores, getStateFromStores) { +function connectToStores(Component, stores, getStateFromStores, options) { class StoreConnector extends ReactComponent { constructor(props, context) { super(props, context); @@ -35,7 +39,6 @@ function connectToStores(Component, stores, getStateFromStores) { this._onStoreChange = this._onStoreChange.bind(this); this.getStateFromStores = this.getStateFromStores.bind(this); this.state = this.getStateFromStores(); - this.wrappedElementRef = createRef(); } getStateFromStores(props) { @@ -51,12 +54,18 @@ function connectToStores(Component, stores, getStateFromStores) { componentDidMount() { this._isMounted = true; - stores.forEach(Store => this.context.getStore(Store).on('change', this._onStoreChange)); + stores.forEach((Store) => + this.context.getStore(Store).on('change', this._onStoreChange) + ); } componentWillUnmount() { this._isMounted = false; - stores.forEach(Store => this.context.getStore(Store).removeListener('change', this._onStoreChange)); + stores.forEach((Store) => + this.context + .getStore(Store) + .removeListener('change', this._onStoreChange) + ); } UNSAFE_componentWillReceiveProps(nextProps) { @@ -64,22 +73,28 @@ function connectToStores(Component, stores, getStateFromStores) { } render() { - const props = (Component.prototype && Component.prototype.isReactComponent) - ? {ref: this.wrappedElementRef} - : null; - return createElement(Component, {...this.props, ...this.state, ...props}); + return createElement(Component, { + ref: this.props.fluxibleRef, + ...this.props, + ...this.state, + }); } } - StoreConnector.displayName = `storeConnector(${Component.displayName || Component.name || 'Component'})`; - StoreConnector.contextType = FluxibleContext; - StoreConnector.WrappedComponent = Component; - - hoistNonReactStatics(StoreConnector, Component); + const forwarded = forwardRef((props, ref) => + createElement(StoreConnector, { + ...props, + fluxibleRef: options?.forwardRef ? ref : null, + }) + ); + forwarded.displayName = `storeConnector(${ + Component.displayName || Component.name || 'Component' + })`; + forwarded.WrappedComponent = Component; - return StoreConnector; + return hoistNonReactStatics(forwarded, Component); } export default connectToStores; diff --git a/packages/fluxible-addons-react/src/provideContext.js b/packages/fluxible-addons-react/src/provideContext.js index a9aa2f45..8c5687ac 100644 --- a/packages/fluxible-addons-react/src/provideContext.js +++ b/packages/fluxible-addons-react/src/provideContext.js @@ -2,7 +2,7 @@ * Copyright 2015, Yahoo Inc. * Copyrights licensed under the New BSD License. See the accompanying LICENSE file for terms. */ -import { Component as ReactComponent, createRef, createElement } from 'react'; +import { Component as ReactComponent, createElement } from 'react'; import { object } from 'prop-types'; import hoistNonReactStatics from 'hoist-non-react-statics'; import { FluxibleProvider } from './FluxibleContext'; @@ -23,20 +23,11 @@ function provideContext(Component) { class ContextProvider extends ReactComponent { constructor(props) { super(props); - this.wrappedElementRef = createRef(); } render() { - const props = - Component.prototype && Component.prototype.isReactComponent - ? { ref: this.wrappedElementRef } - : null; - const { context } = this.props; - const children = createElement(Component, { - ...this.props, - ...props, - }); + const children = createElement(Component, this.props); return createElement(FluxibleProvider, { context }, children); } } diff --git a/packages/fluxible-addons-react/tests/unit/lib/batchedUpdatePlugin.js b/packages/fluxible-addons-react/tests/unit/lib/batchedUpdatePlugin.js index 2fa21d37..ad7b9335 100644 --- a/packages/fluxible-addons-react/tests/unit/lib/batchedUpdatePlugin.js +++ b/packages/fluxible-addons-react/tests/unit/lib/batchedUpdatePlugin.js @@ -1,96 +1,117 @@ /* globals describe, it, afterEach, beforeEach, document */ /* eslint react/prop-types:0 react/no-render-return-value:0 */ import { expect } from 'chai'; +import sinon from 'sinon'; +import TestRenderer from 'react-test-renderer'; import React from 'react'; import ReactDOM from 'react-dom'; -import ReactTestUtils from 'react-dom/test-utils'; -import { JSDOM } from 'jsdom'; import Fluxible from 'fluxible'; -import { batchedUpdatePlugin, connectToStores, provideContext } from '../../../'; +import { + batchedUpdatePlugin, + connectToStores, + provideContext, +} from '../../../'; import FooStore from '../../fixtures/stores/FooStore'; import BarStore from '../../fixtures/stores/BarStore'; +class DumbComponent extends React.Component { + componentDidUpdate() { + this.props.spy(); + } + render() { + return null; + } +} + +const createContext = (plugins = []) => { + const app = new Fluxible({ stores: [FooStore, BarStore] }); + + plugins.forEach((plugin) => app.plug(plugin)); + + return app.createContext(); +}; + +const createComponent = (context) => { + const WrappedComponent = provideContext( + connectToStores(DumbComponent, [FooStore, BarStore], (context) => ({ + foo: context.getStore(FooStore).getFoo(), + bar: context.getStore(BarStore).getBar(), + })) + ); + + const props = { + spy: sinon.stub(), + context: context.getComponentContext(), + }; + + const renderer = TestRenderer.create(); + + const component = renderer.root.findByType(DumbComponent).instance; + + return { component, spy: props.spy }; +}; + describe('fluxible-addons-react', () => { describe('batchedUpdatePlugin', () => { - let appContext; + let clock; beforeEach(() => { - const jsdom = new JSDOM(''); - global.window = jsdom.window; - global.document = jsdom.window.document; - global.navigator = jsdom.window.navigator; - - const app = new Fluxible({ - stores: [FooStore, BarStore] - }); - app.plug(batchedUpdatePlugin()); - - appContext = app.createContext(); + clock = sinon.useFakeTimers(); + sinon + .stub(ReactDOM, 'unstable_batchedUpdates') + .callsFake(TestRenderer.unstable_batchedUpdates); }); afterEach(() => { - delete global.window; - delete global.document; - delete global.navigator; + clock.restore(); + ReactDOM.unstable_batchedUpdates.restore(); }); - it('should only call render once when two stores emit changes', (done) => { - let i = 0; - class Component extends React.Component { - componentDidUpdate() { - i++; - } - render() { - return ( -
- {this.props.foo} - {this.props.bar} -
- ); - } - } - - const WrappedComponent = provideContext(connectToStores(Component, [FooStore, BarStore], (context) => ({ - foo: context.getStore(FooStore).getFoo(), - bar: context.getStore(BarStore).getBar() - }))); - - const container = document.createElement('div'); - const context = appContext.getComponentContext(); - const component = ReactDOM.render(, container); - const wrappedElement = component.wrappedElementRef.current.wrappedElementRef.current; + it('can mock unstable_batchedUpdates', () => { + const context = createContext(); + const { component, spy } = createComponent(context); + + component.setState({ foo: 'far', bar: 'baz' }); + component.setState({ foo: 'far', bar: 'baz' }); + + expect(spy.callCount).to.equal(2); + + spy.reset(); ReactDOM.unstable_batchedUpdates(() => { - wrappedElement.setState({ - foo: 'far', - bar: 'baz' - }); - wrappedElement.setState({ - foo: 'far', - bar: 'baz' - }); + component.setState({ foo: 'far', bar: 'baz' }); + component.setState({ foo: 'far', bar: 'baz' }); }); - expect(i).to.equal(1); - i = 0; + expect(spy.callCount).to.equal(1); + }); - appContext.executeAction((actionContext) => { - actionContext.dispatch('DOUBLE_UP'); - }); + it('updates component only once when two stores emit changes', () => { + const plugins = [batchedUpdatePlugin()]; + const context = createContext(plugins); + const { component, spy } = createComponent(context); + + context.executeAction((context) => context.dispatch('DOUBLE_UP')); + + clock.tick(1); + + expect(component.props.bar).to.equal('bazbaz'); + expect(component.props.foo).to.equal('barbar'); + expect(spy.callCount).to.equal(1); + }); + + it('updates component twice if plugin is not used', () => { + const context = createContext(); + const { component, spy } = createComponent(context); + + context.executeAction((context) => context.dispatch('DOUBLE_UP')); + + clock.tick(1); - (function checkForFinalState() { - const props = wrappedElement.props; - if (props && props.foo === 'barbar' && props.bar === 'bazbaz') { - if (i > 1) { - done(new Error(`Update called ${i} times during dispatch`)); - return; - } - done(); - } else { - setTimeout(checkForFinalState, 5); - } - })(); + expect(component.props.bar).to.equal('bazbaz'); + expect(component.props.foo).to.equal('barbar'); + expect(spy.callCount).to.equal(2); }); }); }); diff --git a/packages/fluxible-addons-react/tests/unit/lib/connectToStores.js b/packages/fluxible-addons-react/tests/unit/lib/connectToStores.js index d1a2f7f9..312ad4a6 100644 --- a/packages/fluxible-addons-react/tests/unit/lib/connectToStores.js +++ b/packages/fluxible-addons-react/tests/unit/lib/connectToStores.js @@ -1,137 +1,157 @@ -/* globals describe, it, afterEach, beforeEach, document */ -/* eslint react/prop-types:0, react/no-render-return-value:0, react/no-find-dom-node:0 */ +/* eslint-disable react/prop-types */ import { expect } from 'chai'; -import React from 'react'; -import ReactDOM from 'react-dom'; -import ReactTestUtils from 'react-dom/test-utils'; -import PropTypes from 'prop-types'; -import { JSDOM } from 'jsdom'; +import React, { forwardRef, useImperativeHandle } from 'react'; +import TestRenderer from 'react-test-renderer'; import createMockComponentContext from 'fluxible/utils/createMockComponentContext'; -import { connectToStores, provideContext, FluxibleContext } from '../../../'; +import { connectToStores, FluxibleComponent } from '../../../'; import FooStore from '../../fixtures/stores/FooStore'; import BarStore from '../../fixtures/stores/BarStore'; -describe('fluxible-addons-react', () => { - describe('connectToStores', () => { - let appContext; +const DumbComponent = ({ foo, bar, onClick }) => ( +
+ {foo} + {bar} +
+); +DumbComponent.displayName = 'DumbComponent'; +DumbComponent.initAction = () => {}; + +const stores = [FooStore, BarStore]; + +const getStateFromStores = ({ getStore, executeAction }) => ({ + foo: getStore(FooStore).getFoo(), + bar: getStore(BarStore).getBar(), + onClick: () => executeAction((context) => context.dispatch('DOUBLE_UP')), +}); - beforeEach(() => { - const jsdom = new JSDOM(''); - global.window = jsdom.window; - global.document = jsdom.window.document; - global.navigator = jsdom.window.navigator; +const ConnectedComponent = connectToStores( + DumbComponent, + stores, + getStateFromStores +); - appContext = createMockComponentContext({ - stores: [FooStore, BarStore] - }); - }); +const renderComponent = (Component, ref) => { + const context = createMockComponentContext({ stores }); - afterEach(() => { - delete global.window; - delete global.document; - delete global.navigator; - }); + const app = TestRenderer.create( + + + + ); - it('should get the state from the stores', (done) => { - class Component extends React.Component { + return { app, context }; +}; +const getComponent = (app) => app.root.findByType(DumbComponent); - constructor() { - super(); - this.onClick = this.onClick.bind(this); - } - - onClick() { - this.context.executeAction((actionContext) => actionContext.dispatch('DOUBLE_UP')); - } - - render() { - return ( -
- {this.props.foo} - {this.props.bar} -
- ); - } - } - Component.contextType = FluxibleContext +describe('fluxible-addons-react', () => { + describe('connectToStores', () => { + it('should hoist and set static properties properly', () => { + expect(ConnectedComponent.displayName).to.equal( + 'storeConnector(DumbComponent)' + ); + expect(ConnectedComponent.WrappedComponent).to.equal(DumbComponent); + expect(ConnectedComponent.initAction).to.be.a('function'); + expect(ConnectedComponent.initAction).to.equal( + DumbComponent.initAction + ); + }); - const WrappedComponent = provideContext(connectToStores(Component, [FooStore, BarStore], (context) => ({ - foo: context.getStore(FooStore).getFoo(), - bar: context.getStore(BarStore).getBar() - }))); + it('should register/unregister from stores on mount/unmount', () => { + const { app, context } = renderComponent(ConnectedComponent); - const container = document.createElement('div'); - const component = ReactDOM.render(, container); - const domNode = ReactDOM.findDOMNode(component); - expect(domNode.querySelector('#foo').textContent).to.equal('bar'); - expect(domNode.querySelector('#bar').textContent).to.equal('baz'); + const barStore = context.getStore(BarStore); + const fooStore = context.getStore(FooStore); - ReactTestUtils.Simulate.click(domNode.querySelector('#button')); + expect(barStore.listeners('change').length).to.equal(1); + expect(fooStore.listeners('change').length).to.equal(1); - expect(domNode.querySelector('#foo').textContent).to.equal('barbar'); - expect(domNode.querySelector('#bar').textContent).to.equal('bazbaz'); + app.unmount(); - expect(appContext.getStore(BarStore).listeners('change').length).to.equal(1); - expect(appContext.getStore(FooStore).listeners('change').length).to.equal(1); + expect(barStore.listeners('change').length).to.equal(0); + expect(fooStore.listeners('change').length).to.equal(0); + }); - ReactDOM.unmountComponentAtNode(container); + it('should forward props from getStateFromStores to component', () => { + const { app } = renderComponent(ConnectedComponent); + const component = getComponent(app); - expect(appContext.getStore(BarStore).listeners('change').length).to.equal(0); - expect(appContext.getStore(FooStore).listeners('change').length).to.equal(0); - done(); + expect(component.props.foo).to.equal('bar'); + expect(component.props.bar).to.equal('baz'); + expect(component.props.onClick).to.be.a('function'); }); - describe('refs', () => { - const hasWrappedComponentRef = component => { - const contextProvider = component; - const storeConnector = contextProvider.wrappedElementRef.current; - const wrappedElement = storeConnector.wrappedElementRef.current; - return Boolean(wrappedElement); - }; - - it('should add a ref to class components', () => { - class Component extends React.Component { - render() { - return