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 1 commit into
base: master
Choose a base branch
from

Conversation

eps1lon
Copy link
Contributor

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

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
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this seems like a great fix.

packages/enzyme-adapter-utils/src/createMountWrapper.jsx Outdated Show resolved Hide resolved
@@ -129,7 +129,7 @@ class ReactThirteenAdapter extends EnzymeAdapter {
Component: type,
props,
context,
...(ref && { ref }),
...(ref && { forwardedRef: ref }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@ljharb ljharb May 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants