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

Freeze ROOT_NODES for child wrappers #1811

Merged

Conversation

jgzuke
Copy link
Collaborator

@jgzuke jgzuke commented Sep 8, 2018

Fixes the tests in #1781.

There are a few alternative ways to do fix those tests (and some things wrong with this one)

Solution 1 - This one! Save the original ROOT node tree, and use in calls to parents etc

  • The saved root node tree will not actually match the ROOT node tree after root update. updates will affect the ROOT node tree but not the saved tree. This is already sort of the case, currently updates to the ROOT (for example setting state) would not affect childWrapper[NODES], but would modify childWrapper[ROOT][NODES]. Given that we want child wrappers to be immutable I think this might make the most sense.
  • It is possible here for something to still accidentally compare between the old and new tree by using .root().getNodeInternal() instead of getRootNodeInternal()

Solution 2 - On update we swap all child wrappers ROOTs to a copy of the old root.

  • Calling and methods that update the parent would no longer work after an update. Ie
const child = wrapper.find(Child);
wrapper.find(...) // Or anything that calls update on the wrapper
child.simulate(...) // Would affect the copy, not the real wrapper

Solution 3 - Make child wrappers not able to cause updates

  • Breaking change, makes simulating child events and setting child state etc not possible

Solution 4 - Make child wrappers not immutable and update with component updates

  • 🤷‍♂️

@ljharb

@jgzuke jgzuke requested a review from ljharb September 8, 2018 06:40
@jgzuke
Copy link
Collaborator Author

jgzuke commented Sep 10, 2018

@ljharb how to rerun tests if they seem flaky?

@ljharb
Copy link
Member

ljharb commented Sep 11, 2018

@jgzuke you should be able to click through to the job and rerun the specific builds that errored; i've just done it for this one.

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 it could be rebased on top of #1781?

packages/enzyme/src/ReactWrapper.js Show resolved Hide resolved
@jgzuke jgzuke changed the base branch from master to fix_find_statefulness September 12, 2018 03:23
@jgzuke
Copy link
Collaborator Author

jgzuke commented Sep 12, 2018

@ljharb switched root to #1781 so they can be merged together

packages/enzyme/src/ReactWrapper.js Outdated Show resolved Hide resolved
packages/enzyme/src/ShallowWrapper.js Outdated Show resolved Hide resolved
@ljharb ljharb merged commit b2672d7 into enzymejs:fix_find_statefulness Sep 21, 2018
ljharb added a commit that referenced this pull request Oct 5, 2018
 - [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`
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