Fix ref on root component pointing to internal component #2101
Conversation
Fix ref on root component pointing to wrapper Backport root ref fix to older adapters Adjust ref call to work with 0.13 host instances Use ref prop type Check if WrapperComponent supports forwardedRef prop
| @@ -129,7 +129,7 @@ class ReactThirteenAdapter extends EnzymeAdapter { | |||
| Component: type, | |||
| props, | |||
| context, | |||
| ...(ref && { ref }), | |||
| ...(ref && { forwardedRef: ref }), | |||
There was a problem hiding this comment.
what happens when this new version of the 13 adapter interacts with an older version of enzyme, that lacks support for the forwardedRef prop?
There was a problem hiding this comment.
@ljharb Good question. We could use some marker on either createMountWrapper or its return value to indicate if a forwardedRef prop is supported. ReactWrapperComponent.propTypes.forwardedRef !== undefined might be sufficient already but this coupling to propTypes is pretty dangerous since you would expect that you can safely remove it. I would probably just add a static supportsForwardedRefProp or supportedProps field to the create component in createMountWrapper and check that. Sounds good?
There was a problem hiding this comment.
That seems reasonable. The important thing is that "old adapter + new enzyme" and "new adapter + old enzyme" do not break in any way - it's totally fine if a bug is only fixed with "new adapter + new enzyme".
| @@ -125,13 +125,16 @@ class ReactThirteenAdapter extends EnzymeAdapter { | |||
| render(el, context, callback) { | |||
| if (instance === null) { | |||
| const { ref, type, props } = el; | |||
| const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter }); | |||
| const refProp = ReactWrapperComponent.supportsForwardedRef === true | |||
There was a problem hiding this comment.
i'm not sure if this is sufficient - enzyme itself doesn't care about adapter-utils. the issue is that there needs to be a way for new enzyme to opt in to this new behavior, so that by default, it continues to have the old behavior.
There was a problem hiding this comment.
The old behavior is a bug though not just a feature. I don't think I understand the problem with this approach. Older versions of enzyme-adapter-utils will continue to attach the ref to the wrapper component. Only newer versions will correctly forward the ref.
There was a problem hiding this comment.
ReactWrapperComponent.supportsForwardedRef will never not be true though, because enzyme-adapter-utils is a regular dependency of the adapter - so as part of releasing this change, the react 13 adapter, for example, will require a version of adapter-utils that has supportsForwardedRef set to true.
There was a problem hiding this comment.
Which is not ideal but if we document that you need to update two dependencies to get this bugfix I don't see an issue. Unless the 13 adapter doesn't work with new adapter-utils versions at which point we just skip the 13 adapter and mark it as wontfix.
There was a problem hiding this comment.
Sorry again, to clarify:
- to get the bugfix, someone will need to update to enzyme-next, and adapter-next
- with enzyme-current and adapter-next, nothing should change
- with enzyme-next and adapter-current, nothing should change
adapter-next will have always have adapter-utils-next.
There was a problem hiding this comment.
I don't understand how the current approach breaks these combinations. Could you give me a concrete example that would somehow break with the current approach?
There was a problem hiding this comment.
Sure. First, supportsForwardedRef will never be false, because any version of an adapter that has code to check it, will also force a version of adapter-utils that has supportsForwardedRef set to true.
So, enzyme calling render with a new adapter will always use the new logic.
As I was writing this comment, I've realized that nothing in this PR changes enzyme itself, only the adapter and utils. Are there no changes needed in enzyme itself for this bugfix? Are there any observable behaviors that would be broken?
Separately, will your test changes fail with the older adapters? If not, can we add some tests that would?
|
When I opened this I didn't realize how many moving parts are involved for this. Since this already has two workarounds I'm not that motivated to spend more hours on this fix. Since I won't work on this I'm closing for now. Feel free to reopen and finish the work required to get this merged. |
ljharb
force-pushed
the
fix/root-ref
branch
from
7deab45
to
d167e28
Compare
ljharb
force-pushed
the
master
branch
4 times, most recently
from
2227326
to
0d5ead7
Compare
ljharb
force-pushed
the
master
branch
3 times, most recently
from
43eb75e
to
39e6b1f
Compare
When using
mount(<div ref={React.createRef()} />)ref.currentwas pointing to an internal wrapper component.Previous workarounds:
mount(<><div ref={React.createRef()} /></>)mount(<div />).instance()Both workarounds require knowledge of either enzymes API (2) or internals (1). Making this just™ work should be preferred IMO.