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.

Sign up for GitHub

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

Jump to bottom

Fix ref on root component pointing to internal component #2101

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix ref on root component pointing to internal component #2101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/enzyme-adapter-react-13/src/ReactThirteenAdapter.js
View file
Open in desktop
Expand Up @@ -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 •
edited

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?

? 'forwardedRef'
: 'ref';
const wrapperProps = {
Component: type,
props,
context,
...(ref && { ref }),
...(ref && { [refProp]: ref }),
};
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const wrappedEl = React.createElement(ReactWrapperComponent, wrapperProps);
instance = React.render(wrappedEl, domNode);
if (typeof callback === 'function') {
Expand Down
7 changes: 5 additions & 2 deletions packages/enzyme-adapter-react-14/src/ReactFourteenAdapter.js
View file
Open in desktop
Expand Up @@ -105,14 +105,17 @@ class ReactFourteenAdapter extends EnzymeAdapter {
render(el, context, callback) {
if (instance === null) {
const { type, props, ref } = el;
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const refProp = ReactWrapperComponent.supportsForwardedRef === true
? 'forwardedRef'
: 'ref';
const wrapperProps = {
Component: type,
wrappingComponentProps: options.wrappingComponentProps,
props,
context,
...(ref && { refProp: ref }),
...(ref && { [refProp]: ref }),
};
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const wrappedEl = React.createElement(ReactWrapperComponent, wrapperProps);
instance = ReactDOM.render(wrappedEl, domNode);
if (typeof callback === 'function') {
Expand Down
7 changes: 5 additions & 2 deletions packages/enzyme-adapter-react-15.4/src/ReactFifteenFourAdapter.js
View file
Open in desktop
Expand Up @@ -140,14 +140,17 @@ class ReactFifteenFourAdapter extends EnzymeAdapter {
render(el, context, callback) {
if (instance === null) {
const { type, props, ref } = el;
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const refProp = ReactWrapperComponent.supportsForwardedRef === true
? 'forwardedRef'
: 'ref';
const wrapperProps = {
Component: type,
wrappingComponentProps: options.wrappingComponentProps,
props,
context,
...(ref && { refProp: ref }),
...(ref && { [refProp]: ref }),
};
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const wrappedEl = React.createElement(ReactWrapperComponent, wrapperProps);
instance = ReactDOM.render(wrappedEl, domNode);
if (typeof callback === 'function') {
Expand Down
7 changes: 5 additions & 2 deletions packages/enzyme-adapter-react-15/src/ReactFifteenAdapter.js
View file
Open in desktop
Expand Up @@ -140,14 +140,17 @@ class ReactFifteenAdapter extends EnzymeAdapter {
render(el, context, callback) {
if (instance === null) {
const { type, props, ref } = el;
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const refProp = ReactWrapperComponent.supportsForwardedRef === true
? 'forwardedRef'
: 'ref';
const wrapperProps = {
Component: type,
wrappingComponentProps: options.wrappingComponentProps,
props,
context,
...(ref && { refProp: ref }),
...(ref && { [refProp]: ref }),
};
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const wrappedEl = React.createElement(ReactWrapperComponent, wrapperProps);
instance = ReactDOM.render(wrappedEl, domNode);
if (typeof callback === 'function') {
Expand Down
7 changes: 5 additions & 2 deletions packages/enzyme-adapter-react-16.1/src/ReactSixteenOneAdapter.js
View file
Open in desktop
Expand Up @@ -273,14 +273,17 @@ class ReactSixteenOneAdapter extends EnzymeAdapter {
render(el, context, callback) {
if (instance === null) {
const { type, props, ref } = el;
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const refProp = ReactWrapperComponent.supportsForwardedRef === true
? 'forwardedRef'
: 'ref';
const wrapperProps = {
Component: type,
props,
wrappingComponentProps,
context,
...(ref && { refProp: ref }),
...(ref && { [refProp]: ref }),
};
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const wrappedEl = React.createElement(ReactWrapperComponent, wrapperProps);
instance = hydrateIn
? ReactDOM.hydrate(wrappedEl, domNode)
Expand Down
7 changes: 5 additions & 2 deletions packages/enzyme-adapter-react-16.2/src/ReactSixteenTwoAdapter.js
View file
Open in desktop
Expand Up @@ -275,14 +275,17 @@ class ReactSixteenTwoAdapter extends EnzymeAdapter {
render(el, context, callback) {
if (instance === null) {
const { type, props, ref } = el;
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const refProp = ReactWrapperComponent.supportsForwardedRef === true
? 'forwardedRef'
: 'ref';
const wrapperProps = {
Component: type,
props,
wrappingComponentProps,
context,
...(ref && { refProp: ref }),
...(ref && { [refProp]: ref }),
};
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const wrappedEl = React.createElement(ReactWrapperComponent, wrapperProps);
instance = hydrateIn
? ReactDOM.hydrate(wrappedEl, domNode)
Expand Down
7 changes: 5 additions & 2 deletions packages/enzyme-adapter-react-16.3/src/ReactSixteenThreeAdapter.js
View file
Open in desktop
Expand Up @@ -294,14 +294,17 @@ class ReactSixteenThreeAdapter extends EnzymeAdapter {
render(el, context, callback) {
if (instance === null) {
const { type, props, ref } = el;
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const refProp = ReactWrapperComponent.supportsForwardedRef === true
? 'forwardedRef'
: 'ref';
const wrapperProps = {
Component: type,
props,
wrappingComponentProps,
context,
...(ref && { refProp: ref }),
...(ref && { [refProp]: ref }),
};
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const wrappedEl = React.createElement(ReactWrapperComponent, wrapperProps);
instance = hydrateIn
? ReactDOM.hydrate(wrappedEl, domNode)
Expand Down
7 changes: 5 additions & 2 deletions packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js
View file
Open in desktop
Expand Up @@ -423,14 +423,17 @@ class ReactSixteenAdapter extends EnzymeAdapter {
return wrapAct(() => {
if (instance === null) {
const { type, props, ref } = el;
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const refProp = ReactWrapperComponent.supportsForwardedRef === true
? 'forwardedRef'
: 'ref';
const wrapperProps = {
Component: type,
props,
wrappingComponentProps,
context,
...(ref && { refProp: ref }),
...(ref && { [refProp]: ref }),
};
const ReactWrapperComponent = createMountWrapper(el, { ...options, adapter });
const wrappedEl = React.createElement(ReactWrapperComponent, wrapperProps);
instance = hydrateIn
? ReactDOM.hydrate(wrappedEl, domNode)
Expand Down
7 changes: 5 additions & 2 deletions packages/enzyme-adapter-utils/src/createMountWrapper.jsx
View file
Open in desktop
Expand Up @@ -69,11 +69,11 @@ export default function createMountWrapper(node, options = {}) {
}

render() {
const { Component, refProp } = this.props;
const { Component, forwardedRef } = this.props;
const { mount, props, wrappingComponentProps } = this.state;
if (!mount) return null;
// eslint-disable-next-line react/jsx-props-no-spreading
const component = <Component ref={refProp} {...props} />;
const component = <Component ref={forwardedRef} {...props} />;
if (WrappingComponent) {
return (
// eslint-disable-next-line react/jsx-props-no-spreading
Expand All @@ -85,17 +85,20 @@ export default function createMountWrapper(node, options = {}) {
return component;
}
}
WrapperComponent.supportsForwardedRef = true;
WrapperComponent.propTypes = {
Component: makeValidElementType(adapter).isRequired,
refProp: PropTypes.oneOfType([PropTypes.string, ref()]),
props: PropTypes.object.isRequired,
wrappingComponentProps: PropTypes.object,
context: PropTypes.object,
forwardedRef: ref(),
};
WrapperComponent.defaultProps = {
refProp: null,
context: null,
wrappingComponentProps: null,
forwardedRef: undefined,
};

if (options.context && (node.type.contextTypes || options.childContextTypes)) {
Expand Down
6 changes: 6 additions & 0 deletions packages/enzyme-test-suite/test/ReactWrapper-spec.jsx
View file
Open in desktop
Expand Up @@ -107,8 +107,14 @@ describeWithDOM('mount', () => {
describeWithDOM('refs', () => {
it('calls ref', () => {
const spy = sinon.spy();

mount(<div ref={spy} />);

expect(spy).to.have.property('callCount', 1);

const [instance] = spy.firstCall.args;
const element = is('<= 0.13') ? instance.getDOMNode() : instance;
expect(element).to.be.instanceOf(global.HTMLElement);
});

/* global HTMLElement */
Expand Down