[New] mount: .state()/.setState(): allow calling on children
#1802
Conversation
ljharb
requested review from
nfcampos,
lelandrichardson,
madicap,
blainekasten,
jgzuke,
aweary,
brucewpaul and
a team
packages/enzyme/src/ReactWrapper.js
Outdated
| * | ||
| * @returns {ReactWrapper} | ||
| */ | ||
| update() { | ||
| if (this[ROOT] !== this) { | ||
| throw new Error('ReactWrapper::update() can only be called on the root'); | ||
| return this[ROOT].update(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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(?
There was a problem hiding this comment.
That's a great idea; i'll look into doing that separately.
packages/enzyme/src/ReactWrapper.js
Outdated
| * | ||
| * @returns {ReactWrapper} | ||
| */ | ||
| update() { | ||
| if (this[ROOT] !== this) { | ||
| throw new Error('ReactWrapper::update() can only be called on the root'); | ||
| return this[ROOT].update(); |
There was a problem hiding this comment.
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
force-pushed
the
mount_state_children
branch
2 times, most recently
from
d7f63b9
to
44a9e54
Compare
ljharb
force-pushed
the
mount_state_children
branch
2 times, most recently
from
a59cb08
to
6d1a498
Compare
- [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`
|
WOW lovely. |
Fixes #635. Fixes #1289.