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 node resolving in React 16 adapter #1169

Merged
merged 4 commits into from
Sep 30, 2017
Merged

Fix node resolving in React 16 adapter #1169

merged 4 commits into from
Sep 30, 2017

Conversation

gregberge
Copy link
Contributor

I added a test relative to #1163 (comment)

I tried to fix it but I couldn't figure out when to use alternate node.

@ljharb ljharb added the v3 bugs label Sep 27, 2017
@gregberge
Copy link
Contributor Author

@lelandrichardson It is probably not the cleaner way to do it but it fixes a huge bug. I think merging it and publishing a patch would be great!

@gregberge gregberge changed the title Add test to reproduce props bug in React 16 Fix node resolving in React 16 adapter Sep 29, 2017
@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2017

Do you have a plan for what we need to expose on React side to remove this hack?

@marvinpinto
Copy link

I was running into a an issue similar to #1153 so I followed @gaearon's advice and tried this patch out. It seemed to do the trick for me 👍

I had to sprinkle wrapper.update() after dom-manipulating events and it worked fine for the most part.

Here is what I did to test this locally:

  • Cloned https://github.com/neoziro/enzyme.git
  • Switched to the react-16-updated-props branch.
  • Inside the cloned enzyme repo: lerna bootstrap && lerna run build
  • Then replaced the enzyme, enzyme-adapter-react-16, and enzyme-adapter-utils packages (in my node_modules directory) with the newly built lerna alternatives (in the packages directory).

I ran into an oddity where enzyme was throwing the following error:

Enzyme Internal Error: configured enzyme adapter did not inherit from the EnzymeAdapter base class

I assumed it was something weird related to how I built this package and so I just commented out the relevant lines in validateAdapter.js - https://github.com/neoziro/enzyme/blob/react-16-updated-props/packages/enzyme/src/validateAdapter.js#L16-L21

@aweary
Copy link
Collaborator

aweary commented Sep 30, 2017

Do you have a plan for what we need to expose on React side to remove this hack?

@gaearon ideally React would expose a way to call toTree on an instance. This logic is already duplicated from the test renderer, so unifying and exporting it could mean that future adapters don't need to implement toTree themselves.

@lelandrichardson
Copy link
Collaborator

@neoziro thanks for hitting this! I think this is good and am going to merge. It puts us in a strictly better state than where we are now.

That said, I'm definitely interested in hashing out more details with @aweary and @gaearon around getting the react 16 adapter to a state where it doesn't depend on any internals. I agree with Brandon that this likely doesn't need to be much more than just exposing a ReactTestUtils.toTree(instance) or something like this. The function itself is kind of already built for the test renderer, but isn't publicly exposed.

Perhaps this wednesday (at react wednesday) could be a good time to hash out some of these details.

In the mean time, I'm going to merge this to get some people unblocked.

Oh, also, @marvinpinto the error you got is indicative of multiple copies of enzyme being installed in your node_modules directory, thus instanceof doesn't match between them.

@lelandrichardson lelandrichardson merged commit 6b0d1f1 into enzymejs:master Sep 30, 2017
@gregberge
Copy link
Contributor Author

@lelandrichardson you are welcome! It was a pleasure to help!

@gregberge gregberge deleted the react-16-updated-props branch September 30, 2017 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants