Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ref on root component pointing to internal component #2101

Open
wants to merge 7 commits into
base: master
from

Conversation

@eps1lon
Copy link
Contributor

commented Apr 16, 2019

When using mount(<div ref={React.createRef()} />) ref.current was pointing to an internal wrapper component.

Previous workarounds:

  1. mount(<><div ref={React.createRef()} /></>)
  2. mount(<div />).instance()

Both workarounds require knowledge of either enzymes API (2) or internals (1). Making this just™ work should be preferred IMO.

@ljharb
Copy link
Member

left a comment

Thanks, this seems like a great fix.

@@ -129,7 +129,7 @@ class ReactThirteenAdapter extends EnzymeAdapter {
Component: type,
props,
context,
...(ref && { ref }),
...(ref && { forwardedRef: ref }),

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 22, 2019

Member

what happens when this new version of the 13 adapter interacts with an older version of enzyme, that lacks support for the forwardedRef prop?

This comment has been minimized.

Copy link
@eps1lon

eps1lon Apr 23, 2019

Author Contributor

@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?

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 24, 2019

Member

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".

@eps1lon eps1lon force-pushed the eps1lon:fix/root-ref branch from 60954e3 to c81f699 Apr 25, 2019

@@ -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

This comment has been minimized.

Copy link
@ljharb

ljharb May 5, 2019

Member

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.

This comment has been minimized.

Copy link
@eps1lon

eps1lon May 6, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@ljharb

ljharb May 6, 2019

Member

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.

This comment has been minimized.

Copy link
@eps1lon

eps1lon May 7, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@ljharb

ljharb May 7, 2019

Member

Sorry again, to clarify:

  1. to get the bugfix, someone will need to update to enzyme-next, and adapter-next
  2. with enzyme-current and adapter-next, nothing should change
  3. with enzyme-next and adapter-current, nothing should change

adapter-next will have always have adapter-utils-next.

This comment has been minimized.

Copy link
@eps1lon

eps1lon May 9, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@ljharb

ljharb May 10, 2019

Member

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?

@eps1lon

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

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.

@eps1lon eps1lon closed this May 11, 2019

@ljharb ljharb reopened this May 11, 2019

@ljharb ljharb self-assigned this May 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.