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

[New] mount: .state()/.setState(): allow calling on children #1802

Merged
merged 1 commit into from
Oct 2, 2018

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Sep 3, 2018

Fixes #635. Fixes #1289.

*
* @returns {ReactWrapper}
*/
update() {
if (this[ROOT] !== this) {
throw new Error('ReactWrapper::update() can only be called on the root');
return this[ROOT].update();
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the part i want review on, in particular - is this the best way to solve it? should i instead keep the throw, but explicitly this[ROOT].update() in most of the places that calls this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally I think it makes sense to always update root. I dont see why anyone adding an update call that might happen in a non root node not wanting it not to update the root. We do already have a call to this[ROOT].update() in simulate, if we decide to change this we should update there.

It might be more clear setting const root = ... and operating on that instead of calling this[ROOT].update() though? When reading a function that calls itself I tend to assume it has n levels of recursion, instead of just one fall through, ie I would assume this[ROOT] might itself have a different root.

As an aside, it seems like a lot of things are guarded with if (this[ROOT] !== this), would it make sense to make a rootOnly( similar to single(?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a great idea; i'll look into doing that separately.

packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx Outdated Show resolved Hide resolved
*
* @returns {ReactWrapper}
*/
update() {
if (this[ROOT] !== this) {
throw new Error('ReactWrapper::update() can only be called on the root');
return this[ROOT].update();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally I think it makes sense to always update root. I dont see why anyone adding an update call that might happen in a non root node not wanting it not to update the root. We do already have a call to this[ROOT].update() in simulate, if we decide to change this we should update there.

It might be more clear setting const root = ... and operating on that instead of calling this[ROOT].update() though? When reading a function that calls itself I tend to assume it has n levels of recursion, instead of just one fall through, ie I would assume this[ROOT] might itself have a different root.

As an aside, it seems like a lot of things are guarded with if (this[ROOT] !== this), would it make sense to make a rootOnly( similar to single(?

@ljharb ljharb force-pushed the mount_state_children branch 2 times, most recently from d7f63b9 to 44a9e54 Compare September 6, 2018 20:26
Copy link
Contributor

@madicap madicap left a comment

Choose a reason for hiding this comment

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

I agree with Jason's comments :)

@ljharb ljharb force-pushed the mount_state_children branch 2 times, most recently from a59cb08 to 6d1a498 Compare October 2, 2018 05:57
@ljharb ljharb merged commit 6d1a498 into master Oct 2, 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`
@pgangwani
Copy link

WOW lovely.

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.

4 participants