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] shallow: A nested ForwardRef is not recognized as a valid component by dive() #1832

Merged
merged 2 commits into from
Oct 2, 2018
Merged

[fix] shallow: A nested ForwardRef is not recognized as a valid component by dive() #1832

merged 2 commits into from
Oct 2, 2018

Conversation

uxtechie
Copy link

@uxtechie uxtechie commented Sep 21, 2018 •
edited by ljharb
Loading

Fixes #1830

Other:

  • Added Utils.isCustomComponentElement tests
  • Pass test:all
  • Pass lint

@uxtechie uxtechie mentioned this pull request Sep 21, 2018
Closed
@ljharb ljharb changed the title Bugfix #1830: A nested ForwarRef is not recognized as a valid component by shallow dive() Bugfix #1830: A nested ForwardRef is not recognized as a valid component by shallow dive() Sep 21, 2018
ljharb
ljharb requested changes Sep 21, 2018
View reviewed changes
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.

Let's add the actual test cases you want to work - ie, a .dive() on a nested ForwardRef.

packages/enzyme/src/Utils.js Outdated Show resolved Hide resolved
ljharb
ljharb requested changes Sep 23, 2018
View reviewed changes
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.

This seems like a much larger change than necessary.

I could see adding isCustomComponentElement to the adapter interface, and calling into that with a fallback to the original, but even that seems like more than should be necessary here.

packages/enzyme-adapter-react-16.3/src/ReactSixteenThreeAdapter.js Outdated
@@ -483,6 +484,10 @@ class ReactSixteenThreeAdapter extends EnzymeAdapter {
return typeOfNode(fragment) === Fragment;
}

isForwardRef(forwardRef) {
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 we need to expand the adapter interface, and hardcode the concept of forward refs in it, here.

Copy link
Author

Choose a reason for hiding this comment

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

I agree that isCustomComponentElement can be an adapter competence because its implementation changes between React versions.

The problem is that I have to identify a forwardRef in a robust way and therefore I need to access the isForwardRef dependency of the react-is package available in the current dependencies of the adapter.

A forwardRef component is an object and not a function like other custom components, this is why isCustomComponentElement fails with them.

The current solution just add changes to 16 and 16.3 adapters, it's a similar solution of the current isFragment adapter method present only in the React 16 adapters.

The proposed solution to expand the current interface adapter to include the isCustomComponentElement method add changes to all the adapters but I agree can be a better solution.

Right now I can' t think of a cleaner or more robust solution than the previous ones, I'm sorry.

I wait for you to know your opinion about it.

@uxtechie
Copy link
Author

Hi @ljharb,

Utils.isCustomComponentElement seems used only by ShallowWrapper.dive()

Is it a good idea remove isCustomComponentElement from Utils and call adapter.isCustomComponentElement directly?

@uxtechie
Copy link
Author

Hi @ljharb,

Please, can you update the pull request state? Thanks a lot,

ljharb
ljharb requested changes Oct 1, 2018
View reviewed changes
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.

we can't remove the method from utils without a breaking change; and we can't make dive break with all current adapter versions, so it'd have to retain the fallback.

packages/enzyme-adapter-react-13/src/ReactThirteenAdapter.js Outdated Show resolved Hide resolved
packages/enzyme-adapter-react-16.3/src/ReactSixteenThreeAdapter.js Outdated Show resolved Hide resolved
packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js Outdated Show resolved Hide resolved
packages/enzyme/src/Utils.js Outdated Show resolved Hide resolved
@ljharb ljharb changed the title Bugfix #1830: A nested ForwardRef is not recognized as a valid component by shallow dive() [fix] shallow: A nested ForwardRef is not recognized as a valid component by dive() Oct 2, 2018
ljharb
ljharb approved these changes Oct 2, 2018
View reviewed changes
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 - I've done a bit of tweaking, and I think this is good to go once tests pass.

@ljharb ljharb merged commit d19f13e into enzymejs:master Oct 2, 2018
@uxtechie
Copy link
Author

uxtechie commented Oct 2, 2018

Thanks to you @ljharb

ljharb added a commit that referenced this pull request Oct 4, 2018
@ljharb
[enzyme-adapter-react-{16.1,16.2}] v1.2.1
f2da4cb 
 - [Fix] ensure that `this.state` starts out `null` when unspecified on a custom component (#1832)
 - [deps] update `babel-preset-airbnb`, `chai`, `eslint`, `mocha`, `enzyme-adapter-utils`, `react-is`, `airbnb-js-shims`
ljharb added a commit that referenced this pull request Oct 5, 2018
@ljharb
[enzyme-adapter-react-16.3] v1.3.0
e558ada 
 - [new] add `isCustomComponentElement` (#1832)
 - [fix] `forwardRef`: respect `.displayName` on the forwardRef (#1817)
 - [deps] update `babel-preset-airbnb`, `chai`, `eslint`, `mocha`, `enzyme-adapter-utils`, `react-is`, `airbnb-js-shims`
ljharb added a commit that referenced this pull request Oct 5, 2018
@ljharb
[enzyme-adapter-react-16] v1.6.0
7ad6910 
 - [new] add `isCustomComponentElement` (#1832)
 - [fix] (<= v16.2) ensure that `this.state` starts out `null` when unspecified on a custom component (#1849)
 - [fix] `forwardRef`: respect `.displayName` on the forwardRef (#1817)
 - [deps] update `babel-preset-airbnb`, `chai`, `eslint`, `mocha`, `enzyme-adapter-utils`, `react-is`, `airbnb-js-shims`
ljharb added a commit that referenced this pull request Oct 5, 2018
@ljharb
[enzyme] v3.7.0
8c8840e 
 - [new] `mount`: `.state()`/`.setState()`: allow calling on children (#1802)
 - [new] `configuration`: add `reset`
 - [fix] `makeOptions`: ensure that config-level `attachTo`/`hydrateIn` are inherited into wrapper options (#1836)
 - [fix] `shallow`/`Utils`: call into adapter’s `isCustomComponentElement` if present (#1832)
 - [fix] `shallow`/`mount`: throw an explicit error when state is null/undefined
 - [fix] freeze ROOT_NODES for child wrappers (#1811)
 - [fix] `shallow`: `.parents`: ensure that one `.find` call does not affect another (#1781)
 - [fix] `mount`: update after `simulateError` (#1812)
 - [refactor] `mount`/`shallow`: `getElement`: use `this.single`
 - [deps] update `babel-preset-airbnb`, `chai`, `eslint`, `mocha`, `enzyme-adapter-utils`, `react-is`, `airbnb-js-shims`
This was referenced Oct 5, 2018
Open
Merged
Merged
Merged
@dependencies dependencies bot mentioned this pull request Nov 1, 2018
Closed
@dependencies dependencies bot mentioned this pull request Dec 1, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
API: shallow bug package: react adapter: 16 package: react adapter: 16.3 semver: minor New stuff. semver: patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@uxtechie @ljharb