WIP: Feature: Implement forwardRef in wp.element.createHigherOrderComponent #7557

nerrad · 2018-06-27T01:46:48Z

Description

Using React.forwardRef it's possible to forward refs to children components from their parents. This pull seeks to implement this with createHigherOrderComponent so any components wrapped by a HOC created through that will automatically have the ref passed through the HOC to them.

Currently this is a work in progress and it's not working as expected:

I expected that I'd be able to just return the result from forwardRef(forwardedRefComponent) but doing a build with that results in errors in the console when executing the GB editor. Doing forwardRef(forwardedRefComponent).render appears to work well for the built files but there are failing tests so I suspect there's some issues not seen in basic user testing of the editor.
The test I added for verifying the ref is actually forwarded isn't passing so it appears that the isn't actually getting forwarded.

I'm putting this pull up just so others can see the work so far and it might be quickly apparent what needs fixed or whether this is only feasible with some refactoring of other things.

Related issues/comments:

See summary comment regarding findings. Basically we need a decision on whether to proceed (and how to proceed) with finishing off this pull considering the findings or whether individual HOCs should be responsible for implementing forwardRef.

How has this been tested?

Trying various blocks in the editor
Added unit tests.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

Rollout

Here's some things needing done in this branch before its ready for merge.

All tests that fail in this branch (because of lack of enzyme support) switched to use alternative test utilities in pulls off master (see pulls linked through this issue).
Master merged in and unit-tests passing.
Jest updated so that snapshots can be updated (currently they don't print the pretty display name because the current Jest version doesn't detect forwardRef wrapped components properly).
Making any necessary changes to existing HOCS implementing forwardRef manually.

nerrad · 2018-06-27T02:38:14Z

Okay in this 02f163e I tried passing through the ref on the OriginalComponent before its enhanced. The test for the forwardRef prop now passes and a build still appears to work okay, but there are still some failing tests elsewhere.

Looks like the tests might just be related to enzyme not working with forwardRef. So reworking those tests to use react-test-renderer may be all that's needed.

nerrad · 2018-06-27T02:42:08Z

It looks like enzyme support for forwardRef might be coming with this pull

gziolo · 2018-06-27T05:36:11Z

@nerrad, I would relay on enzyme anymore, they are very slow with fixing changes introduced with 16.3 and up. They didn't release any new version since January or something like that. We are leaning towards using react-test-renderer. See this commit from @youknowriad cc95059.

nerrad · 2018-06-27T11:42:45Z

I can dig dropping enzyme, but the issue here is that all the failing tests are existing tests using enzyme. So it sounds like all those tests would have to be replaced with react-test-renderer. Should that be done in a separate pull?

nerrad · 2018-06-27T12:53:00Z

@aduth I'm a bit stuck with how we want to forward Refs through the HOCs as a part of this pull and I'm not sure yet I'm on the right approach (or even IF it's a good approach). Currently with the approach I've taken in here, the leaf components would be able to check for a forwardedRef prop and use that for their ref. There's some niggling questions though:

What about existing HOCs like withGlobalEvents which creates its own ref on the wrapped component?
Do we convert existing leaf components (such as what you did here Components: Implement Button as assigning ref via forwardRef #6527) to use the new forwardedRef prop if it exists?
Is this just a higher level extraction so any HOC's using createHigherOrderComponent don't have to do the forwardRef call and can just use forwardRef to assign a ref on WrappedComponents?

nerrad · 2018-06-29T18:34:42Z

I’ve wrestled with this a bit and changed things a bit and tweaked the unit-test that demonstrate one possible use case. It looks like it’d be a bit better than what I originally had, but there’s still some problems.

When using with compose, the “leaf” component would have to use forwardedRef prop directly as opposed to being implemented as the return from forwardRef wrapping it.
createHigherOrderComponent is going to always return a forwardRef symbol. This means that depending on where pure is used in the compose stack, it’s not going to work as expected (and definitely is harder - not sure yet how) to assert if state changes result in rerender.

I’m beginning to wonder whether its a good idea to do forwardRef here and instead just leave it to individual HOC elements to configure. Part of the problem I think is that there’s not really direct access (that I can see) to the passed in component that the “OriginalComponent” is mapped to.

nerrad · 2018-06-29T18:50:28Z

If the current approach is okay, one thing we maybe could do is have createHigherOrderComponent accept a flag to indicate whether it does forwardRef or not ( maybe doForwardRef defaulting to true) and if it isn't then it skips that logic. Then the logic for pure would set that flag to false.

So something like this:

export function createHigherOrderComponent( doForwardRef = true ) {
	return ( mapComponentToEnhancedComponent, modifierName ) => {
		return ( OriginalComponent ) => {
			const WrappedComponent = mapComponentToEnhancedComponent(
				OriginalComponent );
			const EnhancedComponent = doForwardRef ?
				forwardRef(
					( props, ref ) => {
						const forwardedRef = ref !== null ? ref : props.forwardedRef;
						return <WrappedComponent
							{ ...props }
							forwardedRef={ forwardedRef }
						/>;
					}
				) :
				WrappedComponent;
			const { displayName = OriginalComponent.name || 'Component' } = OriginalComponent;
			EnhancedComponent.displayName
				= `${ upperFirst( camelCase( modifierName ) ) }(${ displayName })`;
			return EnhancedComponent;
		};
	};
}


export const pure = createHigherOrderComponent( false )( ( Wrapped ) => {
	if ( Wrapped.prototype instanceof Component ) {
		return class extends Wrapped {
			shouldComponentUpdate( nextProps, nextState ) {
				return ! isShallowEqual( nextProps, this.props ) || ! isShallowEqual( nextState, this.state );
			}
		};
	}

	return class extends Component {
		shouldComponentUpdate( nextProps ) {
			return ! isShallowEqual( nextProps, this.props );
		}

		render() {
			return <Wrapped { ...this.props } />;
		}
	};
}, 'pure' );

The only problem with the above is its a breaking change with the signature change. So this would go back to what I mentioned earlier in the ticket about maybe introducing a new createHigherOrderComponentWithForwardeRef or something like that.

nerrad · 2018-06-29T18:57:11Z

Admittedly, I'm not versed in javascript symbols. So I'll do a bit of reading on it as well.

nerrad · 2018-06-29T20:24:37Z

Alright so the shape of the returned value from forwardRef is simply:

{
  $$typeof: Symbol(react.forward_ref)
 render: [Function]
}

So the symbol is merely used to describe the type of object it is. The important thing remains that forwardRef returns an object with a render function. So it affects HOCs that might do something similar to pure for checking state.

nerrad · 2018-06-29T21:02:28Z

If we do go with the current approach (and work out some solution for testing pure) then leaf components could probably have their definition be something like this:

export const Button = ( props ) => {
    return <button ref={ props.forwardedRef } />;
};

export const ButtonWithRef = forwardRef( ( props, ref ) => {
    const forwardedRef = ref !== null ? ref : props.forwardedRef;
    return <Button { ...props } forwardedRef= { forwardedRef } />;
} );

nerrad · 2018-06-29T21:27:24Z

packages/element/src/test/index.js

+			expect( wrapper.toJSON().children[0] ).toBe( '2' );
+			wrapper.update( <MyComp b /> ); // Changing the prop value should rerender
+			expect( wrapper.toJSON().children[0] ).toBe( '3' );
+			wrapper.root.instance.setState( { a: 1 } ); // New state value should trigger a rerender


this is what breaks on tests because forwardRef returns an object containing a render method. So there is no component instance and thus no state exposed.

nerrad · 2018-06-29T21:31:20Z

I'm wary about using the idea I had for adding an argument to the existing createHigherOrderComponent method for conditionally applying forwardRef. I think it'd be best if we do a new method. From the react docs:

When you start using forwardRef in a component library, you should treat it as a breaking change and release a new major version of your library. This is because your library likely has an observably different behavior (such as what refs get assigned to, and what types are exported), and this can break apps and other libraries that depend on the old behavior.

Conditionally applying React.forwardRef when it exists is also not recommended for the same reasons: it changes how your library behaves and can break your users’ apps when they upgrade React itself.

nerrad · 2018-06-30T14:58:47Z

With 12ad2bc I finally figured out how to extract the WrappedComponent instance for asserting changing state on a Component instance wrapped by pure results in a re-render. This successfully converts the previous pure tests from enzyme to react-test-renderer.

nerrad · 2018-07-01T01:58:39Z

In ed0e374 I've added a test that demonstrates my concern with pure and the changes in this branch. As noted, depending on the order pure is used within compose, it will not handle a class component as expected because it ends up getting passed in to pure as a forwardRef wrapped function component.

nerrad · 2018-07-01T12:55:12Z

So it appears this behaviour of pure is something that already exists in master (obviously). Since I'm not sure whether its "known" or expected behaviour I duplicated the test I added here in a branch of master and created a pull (#7654). That way if that's determined to be invalid, then the same concern I have here might be considered invalid as well. After thinking about it, I think pure is behaving as expected in master. However the issue in this branch is still an issue because the type spit out from createHigherOrderComponent will always be a component wrapped by forwardRef (thus the original class component is not exposed for pure if its part of the compose chain.

nerrad · 2018-07-02T12:53:58Z

What next for this pull?

A summary of some of the things uncovered in this work and what needs a decision (with some potential options).

Implementing `forwardRef` in `createHigherOrderComponent` changes return type.

forwardRef on a component returns essentially a functional component. This affects any code expecting the same type of component returned as what was passed in. While in most cases this might not be problematic, it is technically a breaking change. In the case of pure this is noticeable when pure is used as a part of a composed HOC (eg. withSelect or withDispatch) IF the component being composed is a class component with state.

Things for consideration:

change pure so it is only a HOC that makes functional components "pure" (thus shallow compare on props only) and implement the expectation that if class components with state need to be pure they should extend React.PureComponent. This immediately makes expectations clear and removes the potential for gotchas with usage of compose. Note: recompose has a pure HOC that only applies to functional components (shallow compare on props only).
documentation will need to be clear about what createHigherOrderComponent does (not only composing the higher order component itself but also applying forwardRef.

Enzyme does not support `forwardRef`.

This will mean that there's quite a few tests that will need converted to react-test-renderer. Totally doable and might be a bit better in the long run as react-test-renderer is more low level. However it does make writing tests more difficult.

Introduction of `forwardedRef` prop on all HOCs created with `createHigherOrderComponent`

This means that the composed classes will need to pass through ref (i.e. withGlobalEvents) using the exposed forwardedRef component. Otherwise "leaf" components will need look for the forwardedRef prop.

Inconsistency among HOCs in GB

Currently there's still a few HOCs that don't use createHigherOrderComponent. To be useful, it'd be good to have all the HOC's go through this function. This also means existing HOC's that implement forwardRef individually will need to be updated to implement forwardedRef prop.

aduth · 2018-07-02T15:06:32Z

Thanks for spending the time working through this @nerrad .

One consideration is an extreme idea that's been proposed in passing on a few occasions: Not exposing Component from @wordpress/element, and instead putting full weight into higher-order components as the interface to define enhanced component behaviors. The thought here is to better guarantee future compatibility with e.g. breaking changes of deprecated componentWillMount and other lifecycle methods (source). As it impacts this pull request, if we didn't need to consider pure as operating on state and instead only on props (or general not considering instanceof Component), it shouldn't be as much of an issue? See also withState.

I don't love the idea of allowing a higher-order component to opt-out of ref forwarding, since it makes it less obvious for a developer to know what will happen when they assign a ref. If we can't forward for all, I don't think we should forward for any.

nerrad · 2018-07-03T01:22:05Z

Re not exposing component: In principle I understand that. However practically, I wonder how much benefit that will really bring. Will GB still be "compatible" with custom components written in react by third parties? Will it narrow the use-case for GB components to a WP (or @WordPress package) context? I'm wary of this being a way to make it easier to bridge future compatibility issues with React lifecycles. Would this also mean dropping the use of React.StrictMode.

As it impacts this pull request, if we didn't need to consider pure as operating on state and instead only on props (or general not considering instanceof Component), it shouldn't be as much of an issue? See also withState.

I'm not sure changing wp.element so Component isn't exposed will affect that. I think we can just decide that pure will only make functional components "pure" and then some other interface will be for handling state and props purity. What about introducing a withPureState?

I don't love the idea of allowing a higher-order component to opt-out of ref forwarding, since it makes it less obvious for a developer to know what will happen when they assign a ref. If we can't forward for all, I don't think we should forward for any.

I concur and came to the same conclusion over the course of doing this ticket.

nerrad · 2018-07-04T15:21:45Z

Noting this comment from @gziolo in slack:

@nerrad, I’m a bit late, but I wanted to test performance regression properly. Regarding HOCs, my personal view is that all of them should be created using createHigherOrderComponent helper for consistency even without ref handling. At the moment it standardizes how you can discover those HOCs when using React devTools. Another issue as noted above is that when you compose HOCs you just need to ensure that all of them pass down refs properly. I’d encourage to continue your work and don’t worry as much about breaking changes. I don’t think there are many plugins that use this helper if any. As long as we can ensure that existing hooks like https://github.com/WordPress/gutenberg/blob/master/editor/hooks/custom-class-name.js#L54-L79 work, we can iterate.

React will throw an error for html elements (leaf components) that have a `forwardedRef` prop on them because camelcase props are not allowed.

Note: The snapshot regenerated does look different than the original because creatHigherOrderComponent now wraps in forwardRef. Since Shallow doesn’t render deep, this results in “Component” found in the text for displayName.

used to update snapshot

This also adds mocks for the internal child components that should be tested themselves in isolation. The snapshots are thus updated to reflect the changes with mocks.

aduth

Looking at the actual implementation in createHigherOrderComponent, it seems like the application of ref forwarding is an enhancement atop the enhanced component. In this vein, it seems as though it could be a higher-order component of its own (withForwardedRef). This could make it more straightforward for someone who's opting in to another higher-order component but wants to respect ref assignment. At the same time though, it requires the conscious foresight to understand that ref assignment is otherwise lost in a higher-order component wrapper.

Developer improvement considerations notwithstanding, I also worry about the performance impact. With these changes, each of our higher-order components becomes effectively double-wrapped, correct? Perhaps even triple wrapped, if you consider the third being the result of forwardRef.

aduth · 2018-08-15T17:44:41Z

components/range-control/index.js

+	// components cannot have non native props with camelCase and it'll throw a
+	// warning if present.
+	const forwardedProps = { ...props };
+	delete( forwardedProps.forwardedRef );


FYI, delete is an operator. While this works, as written, it could wrongly imply that we're calling a function. I'd suggest just delete obj.key;.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/delete

You may consider something like Lodash's _.omit to achieve the same effect more succinctly.

This demonstrates why blanket forwarding of props can be faulty vs. explicitly accepting and passing.

You may consider something like Lodash's _.omit to achieve the same effect more succinctly.

Or, for performance sake, to avoid creating a clone twice (which is what's happening with the object spread in the argument destructure), we can include forwardedRef as a destructured prop. This unfortunately requires instructing ESLint that we're intentionally not using a variable.

See also:

gutenberg/packages/components/src/popover/index.js

Lines 227 to 235 in aefe0dd

// Disable reason: We generate the `...contentProps` rest as remainder

// of props which aren't explicitly handled by this component.

/* eslint-disable no-unused-vars */

position,

range,

focusOnMount,

getAnchorRect,

expandOnMobile,

/* eslint-enable no-unused-vars */

aduth · 2018-08-15T17:48:21Z

packages/element/src/index.js

@@ -214,13 +215,18 @@ export { flowRight as compose };
 *
 * @return {WPComponent} Component class with generated display name assigned.
 */
-export function createHigherOrderComponent( mapComponentToEnhancedComponent, modifierName ) {
+export function createHigherOrderComponent( mapComponentToEnhancedComponent,


We should be consistent with arguments placement, either all on one line, or each on their own line.

aduth · 2018-08-15T17:51:09Z

packages/element/src/serialize.js

@@ -224,6 +224,8 @@ const CSS_PROPERTIES_SUPPORTS_UNITLESS = new Set( [
 	'zoom',
 ] );

+const forwardRefSymbol = Symbol.for( 'react.forward_ref' );


This won't work as you expect in IE11. See also #8967 .

aduth · 2018-08-15T17:52:32Z

packages/element/src/serialize.js

@@ -454,6 +456,11 @@ export function renderElement( element, context = {} ) {
 			}

 			return renderElement( tagName( props, context ), context );
+
+		case 'object':


The serializer has since been updated to handle $$typeof, which we can add to for this purpose. See #8189.

aduth · 2018-08-15T17:53:20Z

packages/element/src/serialize.js

@@ -454,6 +456,11 @@ export function renderElement( element, context = {} ) {
 			}

 			return renderElement( tagName( props, context ), context );
+
+		case 'object':
+			if ( tagName.$$typeof && tagName.$$typeof === forwardRefSymbol ) {


FWIW could this be considered a separate enhancement to automatically including forwardRef in createHigherOrderComponent? This seems like something we need to support in the serializer regardless. Maybe worth a separate pull request.

aduth · 2018-08-15T18:00:10Z

blocks/api/registration.js

@@ -55,6 +57,15 @@ const POST_FORMAT_BLOCK_MAP = {
 	video: 'core/video',
 };

+const isEditPropValid = ( settings ) => {


Two things:

This seems like an enhancement we should support separate from forwardRef integration into createHigherOrderComponent, and could be its own pull request.

This is definitely a piercing of the "element" abstraction. It seems like a function which should be offered by @wordpress/element. On glance, it appears similar to but not quite the same as what's already provided through isValidElement. Digging further, I found there is the official react-is module which includes an isValidElementType method, which is essentially identical in purpose to what we have here. We'd have to decide how we want to fit this into our abstraction. Honestly, I'm not sure why there's a React.isValidElement but not also the isValidElementType bundled; they seem appropriate together, and with that in mind I wouldn't mind if our package exported isValidElementType (though leveraging react-is underneath).

aduth · 2018-08-15T18:05:53Z

FWIW, regardless of where this goes, the two separate enhancements identified are valuable discoveries in their own right (#7557 (comment), #7557 (comment)).

gziolo · 2018-08-20T08:31:00Z

Looking at the actual implementation in createHigherOrderComponent, it seems like the application of ref forwarding is an enhancement atop the enhanced component. In this vein, it seems as though it could be a higher-order component of its own (withForwardedRef). This could make it more straightforward for someone who's opting in to another higher-order component but wants to respect ref assignment. At the same time though, it requires the conscious foresight to understand that ref assignment is otherwise lost in a higher-order component wrapper.

This is a very interesting idea. It probably would have to be a wrapper on the compose itself:

composeWithRef( [
    withHOC1,
    withHOC2,
    withHOC3,
    withHOC4,
] )( MyComponent )

which would be internally implemented as something like:

const composeWithRef = ( HOCs) => compose( [
    ( OriginalComponent ) =>
        React.forwardRef( ( props, ref ) => <OriginalComponent { ...props } forwardedRef={ ref } /> ),
    ...HOCs,
    ( OriginalComponent ) =>
        ( { forwardedRef, ...props } ) => <OriginalComponent ref={ forwardedRef } { ...props } />,
] )( MyComponent )

nerrad · 2018-08-20T12:15:27Z

Hmm, ya a withForwardedRef component (or maybe better a variation of the composeWithRef idea @gziolo) is interesting. I guess what route to take comes down to whether GB just wants ref to be exposed automatically on any wrapped leaf components (which was what sparked this pull I think?) or whether it is something that should be only implemented for specific composed components?

The performance question is important and relevant to answering my previous question because wrapping everything with forwardref just so the ref exposed might not be that performant and thus it could be better to just wrap as needed.

nerrad · 2018-08-20T12:16:18Z

FWIW, regardless of where this goes, the two separate enhancements identified are valuable discoveries in their own right (#7557 (comment), #7557 (comment)).

Agreed, and I also agree they could be done as separate pulls (which I'm happy to do when I can carve out some time...hopefully this week).

nerrad · 2018-10-12T17:06:30Z

@aduth, @gziolo - it looks like React.pure will come with forwardRef support built-in in a future version of react. This seems to lend credence to the idea of automatically using forwardRef on HOC creation. I realize GB has its own implementation of pure though.

gziolo

Not that React hooks are about to land, we probably don't have to bother with forwardRef anymore. It also turns out that this isn't something we need to workaround in Gutenberg codebase.

Do you feel like this is something that still needs to be implemented? It definitely got stale and I'm going to add a proper label to make triaging PRs easier in the future.

In my opinion, we should either close it or refresh. The latter might be not efficient though given that React hooks should make HOCs legacy approach.

nerrad · 2019-01-31T18:19:34Z

I'm 👍 for closing.

nerrad self-assigned this Jun 27, 2018

nerrad added [Type] Enhancement A suggestion for improvement. Framework Issues related to broader framework topics, especially as it relates to javascript [Status] In Progress Tracking issues with work in progress labels Jun 27, 2018

nerrad force-pushed the feature/implement-forward-ref-in-hoc branch from d365030 to b48f295 Compare June 29, 2018 10:49

nerrad commented Jun 29, 2018

View reviewed changes

nerrad added [Package] Element /packages/element and removed [Status] In Progress Tracking issues with work in progress labels Jun 30, 2018

nerrad mentioned this pull request Jul 1, 2018

Fix potential flaw in pure Higher Order Component #7654

Closed

4 tasks

nerrad added the Needs Decision Needs a decision to be actionable or relevant label Jul 2, 2018

aduth mentioned this pull request Jul 4, 2018

Modal component #6261

Merged

4 tasks

nerrad force-pushed the feature/implement-forward-ref-in-hoc branch from ed0e374 to b247787 Compare July 4, 2018 15:01

nerrad added 6 commits July 13, 2018 10:23

extract edit prop validation into its own function

139abf3

fix linting issues

434d6aa

account for forwardRef with renderElement serializer

7a5e871

fix conditional

50f3fa9

make sure html elements don’t receive an invalid prop key

e7b36d8

React will throw an error for html elements (leaf components) that have a `forwardedRef` prop on them because camelcase props are not allowed.

only spread props if its spreadable

bc25fac

nerrad force-pushed the feature/implement-forward-ref-in-hoc branch from efc6c7c to bc25fac Compare July 13, 2018 14:43

nerrad added 6 commits July 13, 2018 11:02

update snapshot which shows ForwardRef

55630b6

doh, use correct syntax and improve comments

93ac602

convert test to use react-test-renderer/shallow

82aa887

Note: The snapshot regenerated does look different than the original because creatHigherOrderComponent now wraps in forwardRef. Since Shallow doesn’t render deep, this results in “Component” found in the text for displayName.

convert some test to ‘react-test-renderer/shallow’

6345a1b

used to update snapshot

convert tests away from enzyme

d6069eb

This also adds mocks for the internal child components that should be tested themselves in isolation. The snapshots are thus updated to reflect the changes with mocks.

fix lint issues

16988ee

nerrad mentioned this pull request Jul 18, 2018

Automated Testing: Unskip tests blocked by Enzyme support of React 16.3 #7005

Closed

aduth reviewed Aug 15, 2018

View reviewed changes

ellatrix added [Type] Code Quality Issues or PRs that relate to code quality and removed [Type] Code Quality Issues or PRs that relate to code quality [Type] Refactoring labels Nov 27, 2018

gziolo requested changes Jan 31, 2019

View reviewed changes

gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jan 31, 2019

nerrad closed this Jan 31, 2019

nerrad deleted the feature/implement-forward-ref-in-hoc branch January 31, 2019 18:20

	// Disable reason: We generate the `...contentProps` rest as remainder
	// of props which aren't explicitly handled by this component.
	/* eslint-disable no-unused-vars */
	position,
	range,
	focusOnMount,
	getAnchorRect,
	expandOnMobile,
	/* eslint-enable no-unused-vars */

WIP: Feature: Implement forwardRef in wp.element.createHigherOrderComponent #7557

WIP: Feature: Implement forwardRef in wp.element.createHigherOrderComponent #7557

Conversation

nerrad commented Jun 27, 2018 • edited Loading

Description

How has this been tested?

Checklist:

Rollout

nerrad commented Jun 27, 2018 • edited Loading

nerrad commented Jun 27, 2018

gziolo commented Jun 27, 2018

nerrad commented Jun 27, 2018

nerrad commented Jun 27, 2018 • edited Loading

nerrad commented Jun 29, 2018

nerrad commented Jun 29, 2018 • edited Loading

nerrad commented Jun 29, 2018

nerrad commented Jun 29, 2018

nerrad commented Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

nerrad commented Jun 29, 2018 • edited Loading

nerrad commented Jun 30, 2018 • edited Loading

nerrad commented Jul 1, 2018 • edited Loading

nerrad commented Jul 1, 2018 • edited Loading

nerrad commented Jul 2, 2018 • edited Loading

What next for this pull?

Implementing forwardRef in createHigherOrderComponent changes return type.

Enzyme does not support forwardRef.

Introduction of forwardedRef prop on all HOCs created with createHigherOrderComponent

Inconsistency among HOCs in GB

aduth commented Jul 2, 2018

nerrad commented Jul 3, 2018

nerrad commented Jul 4, 2018

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

aduth commented Aug 15, 2018

gziolo commented Aug 20, 2018

nerrad commented Aug 20, 2018

nerrad commented Aug 20, 2018

nerrad commented Oct 12, 2018

gziolo left a comment

Choose a reason for hiding this comment

nerrad commented Jan 31, 2019

nerrad commented Jun 27, 2018 •

edited

Loading

nerrad commented Jun 27, 2018 •

edited

Loading

nerrad commented Jun 27, 2018 •

edited

Loading

nerrad commented Jun 29, 2018 •

edited

Loading

nerrad commented Jun 29, 2018 •

edited

Loading

nerrad commented Jun 29, 2018 •

edited

Loading

nerrad commented Jun 30, 2018 •

edited

Loading

nerrad commented Jul 1, 2018 •

edited

Loading

nerrad commented Jul 1, 2018 •

edited

Loading

nerrad commented Jul 2, 2018 •

edited

Loading

Implementing `forwardRef` in `createHigherOrderComponent` changes return type.

Enzyme does not support `forwardRef`.

Introduction of `forwardedRef` prop on all HOCs created with `createHigherOrderComponent`

aduth Aug 15, 2018 •

edited

Loading