From 6c36e7480034b1f849435c06e8c83ce103c5ff6d Mon Sep 17 00:00:00 2001 From: Maarten Rijke Date: Mon, 27 May 2019 17:22:52 +0200 Subject: [PATCH] #1211 - Deprecate withRef in favor of forwardRef and React.forwardRef (#1271) * Deprecate withRef in favor of forwardRef and React.forwardRef Deprecate withRef and use forwardRef prop instead. When forwardRef is true, the ref passed to the injected component will be passed down to the wrapped component. Display a invariant message when withRef is used. Closes #1211. * Refactor withIntl to be composable Refactor the call to withIntl to allow for HoC composing, such as: export default compose( connect(...conntextOptions), withIntl(intlOptions) ) This is done without changing the main API, options can still be passed in as second arguments to withIntl too. Add 'react-is' as a strict dependency, change the rollup config to be able to properly build with it. --- package.json | 3 +- rollup.config.dist.js | 1 + rollup.config.lib.js | 2 +- src/components/withIntl.js | 72 +++++---- test/unit/components/withIntl.js | 247 +++++++++++++++++-------------- yarn.lock | 2 +- 6 files changed, 181 insertions(+), 146 deletions(-) diff --git a/package.json b/package.json index 299be80b69..48573db6ac 100644 --- a/package.json +++ b/package.json @@ -72,7 +72,8 @@ "intl-messageformat": "^2.1.0", "intl-relativeformat": "^2.1.0", "invariant": "^2.1.1", - "react-display-name": "^0.2.4" + "react-display-name": "^0.2.4", + "react-is": "^16.3.1" }, "peerDependencies": { "prop-types": "^15.5.4", diff --git a/rollup.config.dist.js b/rollup.config.dist.js index f96ca1dff6..e95e114249 100644 --- a/rollup.config.dist.js +++ b/rollup.config.dist.js @@ -41,6 +41,7 @@ export default { }), commonjs({ sourcemap: true, + namedExports: {'react-is': ['isValidElementType']}, }), replace({ 'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV), diff --git a/rollup.config.lib.js b/rollup.config.lib.js index 2495207e76..d3966bbc77 100644 --- a/rollup.config.lib.js +++ b/rollup.config.lib.js @@ -27,7 +27,7 @@ export default { ], plugins: [ babel(), - commonjs(), + commonjs({namedExports: {'react-is': ['isValidElementType']}}), nodeResolve({ jsnext: true }) diff --git a/src/components/withIntl.js b/src/components/withIntl.js index 333ae18ee4..09148f1e6f 100644 --- a/src/components/withIntl.js +++ b/src/components/withIntl.js @@ -1,4 +1,5 @@ import React, {Component, createContext} from 'react'; +import {isValidElementType} from 'react-is'; import hoistNonReactStatics from 'hoist-non-react-statics'; import invariant from 'invariant'; import {invariantIntlContext} from '../utils'; @@ -8,62 +9,71 @@ function getDisplayName(Component) { } const IntlContext = createContext(null); -const { - Consumer: IntlConsumer, - Provider: IntlProvider -} = IntlContext +const {Consumer: IntlConsumer, Provider: IntlProvider} = IntlContext; -export const Provider = IntlProvider -export const Context = IntlContext +export const Provider = IntlProvider; +export const Context = IntlContext; -export default function withIntl(WrappedComponent, options = {}) { +export default function withIntl(componentOrOptions, options) { + if (isValidElementType(componentOrOptions)) { + // use call to make `options` available on `this` + return createWrapper.call({options}, componentOrOptions); + } + // return a function with `options` bound to `this` + return createWrapper.bind({options: componentOrOptions}); +} + +function createWrapper(WrappedComponent) { + let options = (this && this.options) || {}; const { intlPropName = 'intl', + forwardRef = false, + // DEPRECATED - use forwardRef and ref on injected component withRef = false, - enforceContext = true + enforceContext = true, } = options; - class withIntl extends Component { + invariant( + !withRef, + '[React Intl] withRef and getWrappedInstance() are deprecated, ' + + "instead use the 'forwardRef' option and create a ref directly on the wrapped component." + ); + + class WithIntl extends Component { static displayName = `withIntl(${getDisplayName(WrappedComponent)})`; static WrappedComponent = WrappedComponent; - wrappedInstance = (ref) => { - this.wrappedInstance.current = ref; - } - - getWrappedInstance() { - invariant( - withRef, - '[React Intl] To access the wrapped instance, ' + - 'the `{withRef: true}` option must be set when calling: ' + - '`withIntl()`' - ); - - return this.wrappedInstance.current; - } - - render () { + render() { return ( - {(intl) => { + {intl => { if (enforceContext) { - invariantIntlContext({ intl }); + invariantIntlContext({intl}); } return ( ); }} - ) + ); } } - return hoistNonReactStatics(withIntl, WrappedComponent); + if (forwardRef) { + return hoistNonReactStatics( + React.forwardRef((props, ref) => ( + + )), + WrappedComponent + ); + } + + return hoistNonReactStatics(WithIntl, WrappedComponent); } diff --git a/test/unit/components/withIntl.js b/test/unit/components/withIntl.js index b1079498a3..62aaa2b853 100644 --- a/test/unit/components/withIntl.js +++ b/test/unit/components/withIntl.js @@ -2,144 +2,167 @@ import expect, {spyOn} from 'expect'; import React from 'react'; import {mount} from 'enzyme'; import {intlShape} from '../../../src/types'; -import IntlProvider from '../../../src/components/provider' +import IntlProvider from '../../../src/components/provider'; import withIntl from '../../../src/components/withIntl'; -const mountWithProvider = (el) => mount( - - { el } - -) +const mountWithProvider = el => + mount({el}); describe('withIntl()', () => { - let Wrapped; - let rendered; - - beforeEach(() => { - Wrapped = () =>
; - Wrapped.displayName = 'Wrapped'; - Wrapped.propTypes = { - intl: intlShape.isRequired, - }; - Wrapped.someNonReactStatic = { - foo: true - }; - rendered = null; + let Wrapped; + let rendered; + + beforeEach(() => { + Wrapped = () =>
; + Wrapped.displayName = 'Wrapped'; + Wrapped.propTypes = { + intl: intlShape.isRequired, + }; + Wrapped.someNonReactStatic = { + foo: true, + }; + rendered = null; + }); + + afterEach(() => { + rendered && rendered.unmount(); + }); + + it('allows introspection access to the wrapped component', () => { + expect(withIntl(Wrapped).WrappedComponent).toBe(Wrapped); + }); + + it('hoists non-react statics', () => { + expect(withIntl(Wrapped).someNonReactStatic.foo).toBe(true); + }); + + describe('displayName', () => { + it('is descriptive by default', () => { + expect(withIntl(() => null).displayName).toBe('withIntl(Component)'); }); - afterEach(() => { - rendered && rendered.unmount(); - }) - - it('allows introspection access to the wrapped component', () => { - expect(withIntl(Wrapped).WrappedComponent).toBe(Wrapped); + it("includes `WrappedComponent`'s `displayName`", () => { + Wrapped.displayName = 'Foo'; + expect(withIntl(Wrapped).displayName).toBe('withIntl(Foo)'); }); + }); + + it('throws when is missing from ancestry', () => { + const Injected = withIntl(Wrapped); + + const consoleError = spyOn(console, 'error'); // surpress console error from JSDom + expect(() => (rendered = mount())).toThrow( + '[React Intl] Could not find required `intl` object. needs to exist in the component ancestry.' + ); + consoleError.restore(); + }); + + it('renders with `intl` prop', () => { + const Injected = withIntl(Wrapped); + + rendered = mountWithProvider(); + const wrappedComponent = rendered.find(Wrapped); + // React 16 renders different in the wrapper + const intlProvider = rendered.find(IntlProvider).childAt(0); + + expect(wrappedComponent.prop('intl')).toBe( + intlProvider.instance().getContext() + ); + }); + + it('propagates all props to ', () => { + const Injected = withIntl(Wrapped); + const props = { + foo: 'bar', + }; + + rendered = mountWithProvider(); + const wrappedComponent = rendered.find(Wrapped); + + Object.keys(props).forEach(key => { + expect(wrappedComponent.prop(key)).toBe(props[key]); + }); + }); - it('hoists non-react statics',() => { - expect(withIntl(Wrapped).someNonReactStatic.foo).toBe(true) - }) - - describe('displayName', () => { - it('is descriptive by default', () => { - expect(withIntl(() => null).displayName).toBe('withIntl(Component)'); - }); - - it('includes `WrappedComponent`\'s `displayName`', () => { - Wrapped.displayName = 'Foo'; - expect(withIntl(Wrapped).displayName).toBe('withIntl(Foo)'); + describe('options', () => { + describe('intlPropName', () => { + it("sets 's `props[intlPropName]` to `context.intl`", () => { + const propName = 'myIntl'; + Wrapped.propTypes = { + [propName]: intlShape.isRequired, + }; + const Injected = withIntl(Wrapped, { + intlPropName: propName, }); - }); - it('throws when is missing from ancestry', () => { - const Injected = withIntl(Wrapped); + rendered = mountWithProvider(); + const wrapped = rendered.find(Wrapped); + const intlProvider = rendered.find(IntlProvider).childAt(0); - const consoleError = spyOn(console, 'error'); // surpress console error from JSDom - expect(() => rendered = mount()).toThrow( - '[React Intl] Could not find required `intl` object. needs to exist in the component ancestry.' + expect(wrapped.prop(propName)).toBe( + intlProvider.instance().getContext() ); - consoleError.restore(); + }); }); - it('renders with `intl` prop', () => { - const Injected = withIntl(Wrapped); - - rendered = mountWithProvider(); - const wrappedComponent = rendered.find(Wrapped); - // React 16 renders different in the wrapper - const intlProvider = rendered.find(IntlProvider).childAt(0); - - expect( - wrappedComponent.prop('intl') - ).toBe(intlProvider.instance().getContext()); + describe('withRef', () => { + it('throws when true', () => { + expect(() => withIntl(Wrapped, {withRef: true})).toThrow( + '[React Intl] withRef and getWrappedInstance() are deprecated, ' + + "instead use the 'forwardRef' option and create a ref directly on the wrapped component." + ); + }); }); - it('propagates all props to ', () => { + describe('forwardRef', () => { + it("doesn't forward the ref when forwardRef is `false`", () => { const Injected = withIntl(Wrapped); - const props = { - foo: 'bar' - } + const wrapperRef = React.createRef(); - rendered = mountWithProvider(); - const wrappedComponent = rendered.find(Wrapped); + rendered = mountWithProvider(); + const wrapper = rendered.find(Injected); - Object.keys(props).forEach((key) => { - expect(wrappedComponent.prop(key)).toBe(props[key]); - }) - }); + expect(wrapperRef.current).toBe(wrapper.instance()); + }); - describe('options', () => { - describe('intlPropName', () => { - it('sets \'s `props[intlPropName]` to `context.intl`', () => { - const propName = 'myIntl'; - Wrapped.propTypes = { - [propName]: intlShape.isRequired - }; - const Injected = withIntl(Wrapped, { - intlPropName: propName - }); - - rendered = mountWithProvider(); - const wrapped = rendered.find(Wrapped); - // React 16 renders differently - const intlProvider = React.version.startsWith('16') ? - rendered.find(IntlProvider).childAt(0) : rendered.find(IntlProvider).childAt(0).childAt(0); - - expect(wrapped.prop(propName)).toBe( - intlProvider.instance().getContext() - ); - }); - }); + it('forwards the ref properly to the wrapped component', () => { + Wrapped = class extends React.Component { + render() { + return null; + } + }; + const Injected = withIntl(Wrapped, {forwardRef: true}); + const wrapperRef = React.createRef(); - describe('withRef', () => { - it('throws when `false` and getWrappedInstance() is called', () => { - const Injected = withIntl(Wrapped); + rendered = mountWithProvider(); + const wrapped = rendered.find(Wrapped); - rendered = mountWithProvider(); - const wrapper = rendered.find(Injected); + expect(wrapperRef.current).toBe(wrapped.instance()); + }); + }); - expect(() => wrapper.instance().getWrappedInstance()).toThrow( - '[React Intl] To access the wrapped instance, the `{withRef: true}` option must be set when calling: `withIntl()`' - ); - }); + it('can also be used with a bound function', () => { + Wrapped = class extends React.Component { + render() { + return null; + } + }; + const propName = 'myPropName'; + const customWithIntl = withIntl({ + forwardRef: true, + intlPropName: propName, + }); + const wrapperRef = React.createRef(); + const Injected = customWithIntl(Wrapped); - it('does not throw when `true` getWrappedInstance() is called', () => { - Wrapped = class extends React.Component { - render () { - return null - } - } + rendered = mountWithProvider(); + const wrapped = rendered.find(Wrapped); - const Injected = withIntl(Wrapped, { withRef: true }); + expect(wrapperRef.current).toBe(wrapped.instance()); - rendered = mountWithProvider(); - const wrapper = rendered.find(Injected); - const wrapped = rendered.find(Wrapped); + const intlProvider = rendered.find(IntlProvider).childAt(0); - expect(() => wrapper.instance().getWrappedInstance()) - .toNotThrow(); - expect(wrapper.instance().getWrappedInstance()) - .toBe(wrapped.instance()); - }); - }); + expect(wrapped.prop(propName)).toBe(intlProvider.instance().getContext()); }); + }); }); diff --git a/yarn.lock b/yarn.lock index e25309ad64..ad24e87642 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5954,7 +5954,7 @@ react-dom@^16.3.3: prop-types "^15.6.2" scheduler "^0.13.6" -react-is@^16.7.0, react-is@^16.8.1, react-is@^16.8.6: +react-is@^16.3.1, react-is@^16.7.0, react-is@^16.8.1, react-is@^16.8.6: version "16.8.6" resolved "https://registry.yarnpkg.com/react-is/-/react-is-16.8.6.tgz#5bbc1e2d29141c9fbdfed456343fe2bc430a6a16" integrity sha512-aUk3bHfZ2bRSVFFbbeVS4i+lNPZr3/WM5jT2J5omUVV1zzcs1nAaf3l51ctA5FFvCRbhrH0bdAsRRQddFJZPtA==