add disableLifecycleMethods for shallow #789

jwbay · 2017-01-28T19:09:07Z

As part of the plan to switch on all lifecycle methods for shallow in the next major, introduce a no-op flag ahead of time that turns that off.

Ref: #678 (comment)

~~Since there's no options validation, I'm not sure any code changes are necessary.~~ (added) It may be confusing if someone sets this flag to explicitly false and expects their lifecycle methods to be called. Should we set lifecycleExperimental to true in that case?

I didn't prefix the option with deprecated because it seemed this flag is going to be sticking around as the official, inverted version of lifecycleExperimental. I may have misunderstood something though.

The name feels a bit off since it's not disabling all the lifecycle methods, just two of them. Should it be something like disableFullLifecycle?

ljharb

This should still have some code to ensure that users aren't passing flags both to enable and disable the methods, and throws in that case.

jwbay · 2017-01-28T20:53:05Z

Added some validation.

ljharb · 2017-01-28T21:37:00Z

src/ShallowWrapper.js

+function validateOptions(options) {
+  if (
+    options.lifecycleExperimental != null &&
+    options.lifecycleExperimental === options.disableLifecycleMethods


i think this comparison should probably just ‼ both options, and throw if they're the same, as long as both of them aren't == null?

Makes sense, done

ljharb · 2017-01-28T21:37:54Z

test/ShallowWrapper-spec.jsx

+      expect(() => shallow(<div />, {
+        lifecycleExperimental: false,
+        disableLifecycleMethods: false,
+      })).to.throw(/same value/);


let's also add tests where both are omitted, and where either aren't a boolean (ie, it should throw if a non-boolean non-undefined value is provided)

jwbay · 2017-01-29T05:55:36Z

test/ShallowWrapper-spec.jsx

+          ['componentWillReceiveProps'],
+          ['shouldComponentUpdate'],
+          ['componentWillUpdate'],
+          ['render'],


I'm not sure whether componentDidUpdate is supposed to be called here, but it's not currently. If this behavior is correct it's going to make this flag + documentation pretty confusing since, per next test down, setState does trigger componentDidUpdate for shallow rendered components (even without enzyme in the picture).

The flag is looking more like it should be disableDidMountAllTheTimeAndDidUpdateSomeOfTheTime. 😅

cc @koba04 any thoughts on correctness here? Note this is all without lifecycleExperimental set to true.

@jwbay
Currently, lifecycleExperimental does complement lifecycle methods that ShallowRenderer is missing, which are componentDidMount and componentDidUpdate in setProps and setContext #318 (comment).

In setState, componentDidUpdate is always called because ShallowWrapper calls instance.setState() directly. 😅

To be clear, if disableLifecycleMethods set to true, componentDidMount and componentDidUpdate(in setProps and setContext) are not called on the component.

ljharb

I think this is looking great, but let's get more reviewers to take a look before merging.

koba04 · 2017-01-30T07:26:55Z

src/ShallowWrapper.js

+  const { lifecycleExperimental, disableLifecycleMethods } = options;
+  if (
+    typeof lifecycleExperimental !== 'undefined' &&
+    lifecycleExperimental !== !!lifecycleExperimental


typeof lifecycleExperimental !== 'boolean' is more explicitly?

ljharb · 2017-02-01T05:34:42Z

test/ShallowWrapper-spec.jsx

+        });
+      });
+
+      describeIf(REACT014, 'setContext', () => {


I'm glad we're testing this on all React versions - could you elaborate on why componentWillReceiveProps fires in 13 and 15, but not 14?

In React 13 and 14, the check for whether willReceiveProps is called is just a strict equality check in ReactCompositeComponent between the previous and next elements. In React 15, they added an additional equality check between previous and next context.

willReceiveProps is called in 13 because Enzyme clones the element on each render in react-compat's createRendererCompatible, failing the equality check.

It's not called in 14 because the elements are equal. createRendererCompatible is only used for React 13.

It's called in 15 because of the new check for context.

aweary

I don't really see the advantage of adding a no-op flag in a minor release, @ljharb can you clarify why we'd want that? It feels like it will just be ignored anyway if it's a no-op.

But otherwise this implementation LGTM if we want this.

ljharb · 2017-02-16T04:00:55Z

@aweary because then people who depend on the false behavior have a way to explicitly preserve it when they upgrade (which will flip the default, making true the noop instead of false), without having to change their code.

aweary · 2017-02-16T16:15:11Z

It just sounds unlikely that anyone is going enable a no-op flag from a minor release when their tests are all working as they would expect. What's more likely is that they upgrade to the next major release, have breakage, and then enable the flag.

But either way, it's not like this change will hurt anyone so 👍

ljharb · 2017-02-16T17:02:10Z

I envision "upgrade, breakage, downgrade, gradually migrate their tests to use an explicit flag, upgrade smoothly later" :-)

blainekasten · 2017-03-14T05:59:59Z

Sooo. Sounds like this is good to merge then? @jwbay mind rebasing?

jwbay · 2017-03-15T02:46:12Z

@blainekasten rebased

add missing setContext test

blainekasten · 2017-03-16T14:51:21Z

cc @ljharb do you want these all rebased down to 1 commit or is this fine?

ljharb · 2017-03-17T05:53:49Z

This is fine, as long as there's no merge commits :-)

blainekasten · 2017-03-17T18:50:13Z

Thanks for the work on this @jwbay ! This is big!

jwbay mentioned this pull request Jan 28, 2017

enable lifecycleExperimental by default? #678

Closed

ljharb requested changes Jan 28, 2017

View reviewed changes

jwbay commented Jan 29, 2017

View reviewed changes

ljharb approved these changes Jan 30, 2017

View reviewed changes

ljharb added semver: minor New stuff. API: shallow labels Jan 30, 2017

ljharb requested review from nfcampos, lelandrichardson, blainekasten and aweary January 30, 2017 05:56

koba04 reviewed Jan 30, 2017

View reviewed changes

ljharb reviewed Feb 1, 2017

View reviewed changes

jwbay force-pushed the disableLifecycleMethods branch from 2fd4aee to 027d5a2 Compare February 8, 2017 00:18

aweary approved these changes Feb 15, 2017

View reviewed changes

jwbay force-pushed the disableLifecycleMethods branch from 027d5a2 to 4f34542 Compare March 15, 2017 02:10

blainekasten approved these changes Mar 15, 2017

View reviewed changes

jwbay added 5 commits March 15, 2017 18:12

add disableLifecycleMethods to documentation

8eeb0b7

forbid invalid combinations of lifecycle option flags

cff2ff5

stronger validation for shallow lifecycle flags

9fc50dc

adjust documentation to reflect reality

5dda4f8

add missing setContext test

split setContext test between react versions

765ec95

jwbay force-pushed the disableLifecycleMethods branch from 4f34542 to 765ec95 Compare March 15, 2017 22:13

nfcampos approved these changes Mar 16, 2017

View reviewed changes

blainekasten merged commit 595d15f into enzymejs:master Mar 17, 2017

jwbay deleted the disableLifecycleMethods branch March 17, 2017 19:08

koba04 mentioned this pull request Mar 29, 2017

this.node is undefined in componentWillUnmount() for shallow renderer #865

Closed

jwbay restored the disableLifecycleMethods branch August 13, 2017 16:35

jwbay deleted the disableLifecycleMethods branch August 13, 2017 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add disableLifecycleMethods for shallow #789

add disableLifecycleMethods for shallow #789

jwbay commented Jan 28, 2017 •

edited

Loading

ljharb left a comment

jwbay commented Jan 28, 2017

ljharb Jan 28, 2017

jwbay Jan 29, 2017

ljharb Jan 28, 2017

jwbay Jan 29, 2017

jwbay Jan 29, 2017

koba04 Jan 30, 2017

ljharb left a comment

koba04 Jan 30, 2017

ljharb Feb 1, 2017

jwbay Feb 8, 2017 •

edited

Loading

aweary left a comment

ljharb commented Feb 16, 2017

aweary commented Feb 16, 2017

ljharb commented Feb 16, 2017

blainekasten commented Mar 14, 2017

jwbay commented Mar 15, 2017

blainekasten commented Mar 16, 2017

ljharb commented Mar 17, 2017

blainekasten commented Mar 17, 2017

add disableLifecycleMethods for shallow #789

add disableLifecycleMethods for shallow #789

Conversation

jwbay commented Jan 28, 2017 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

jwbay commented Jan 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwbay Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

aweary left a comment

Choose a reason for hiding this comment

ljharb commented Feb 16, 2017

aweary commented Feb 16, 2017

ljharb commented Feb 16, 2017

blainekasten commented Mar 14, 2017

jwbay commented Mar 15, 2017

blainekasten commented Mar 16, 2017

ljharb commented Mar 17, 2017

blainekasten commented Mar 17, 2017

jwbay commented Jan 28, 2017 •

edited

Loading

jwbay Feb 8, 2017 •

edited

Loading