Implement spyMethod and use it to inspect the result of shouldComponentUpdate #1192

koba04 · 2017-09-28T09:14:43Z

This PR is to follow up #1133 (comment)

…onentUpdate`/`getSnapshotBeforeUpdate`

koba04 · 2017-09-28T09:16:25Z

packages/enzyme/src/ShallowWrapper.js

+          if (props) this[UNRENDERED] = cloneElement(adapter, this[UNRENDERED], props);
+          this[RENDERER].render(this[UNRENDERED], nextContext);
+          if (spy) {
+            shouldRender = spy.getCall(0).returnValue;


Should we care about calling shouldComponentUpdate multiple times?

Maybe it should get the last call, not the first?

ljharb · 2017-09-28T12:18:25Z

packages/enzyme/src/ShallowWrapper.js

+          if (props) this[UNRENDERED] = cloneElement(adapter, this[UNRENDERED], props);
+          this[RENDERER].render(this[UNRENDERED], nextContext);
+          if (spy) {
+            shouldRender = spy.getCall(0).returnValue;


Maybe it should get the last call, not the first?

ljharb · 2017-09-28T12:18:53Z

packages/enzyme/src/ShallowWrapper.js

        }
        instance.setState(state, callback);
+        if (spy) {
+          shouldRender = spy.getCall(0).returnValue;


I've fixed it!

ljharb

Seems good to me!

Before we merge this, can we confirm what the oldest JS engine sinon supports is?

koba04 · 2017-09-29T14:17:09Z

I'm not sure what the oldest JS engine sinon supports is.
But according to the CI results of sinon, sinon supports Node v4 or later.
In browser environment, according to its sourcelabs results, sinon supports IE11.

lelandrichardson · 2017-09-30T17:16:36Z

I'm not sure I agree with this change. This seems like a huge (and often problematic) dependency to take on for very little benefit.

ljharb · 2017-09-30T17:19:16Z

Fair point (not sure about problematic tho; sinon's always worked fine in my experience) the benefit I see is ensuring that the property descriptor remains the same after restoring, which is tedious to do correctly.

lelandrichardson · 2017-09-30T17:21:36Z

Honestly I would prefer that we do our own thing here instead of pulling in sinon. If we need to do this in a more robust way and build a util function, or depend on a package that does just that, then that's fine. I'm also wondering if this would have strange conflicts with people using sinon on their own with shouldComponentUpdate and sinon handling sinon-spied functions in a different way in that case.

lelandrichardson · 2017-09-30T18:16:08Z

@ljharb when i say "problematic" i mean that I know that in the past webpack and browserify both had problems bundling sinon. I think that might have been fixed in a more recent version though.

ljharb · 2017-09-30T19:49:25Z

Those are all valid concerns. Let's definitely hold off on merging this for now.

koba04 · 2017-10-14T10:13:46Z

I'm working on to implement own spyMethod instead of sinon.spy.

koba04 · 2017-10-17T15:37:05Z

I've implemented a custom spy function, which is a minimum implementation needed by enzyme.

ljharb · 2017-10-17T16:32:02Z

packages/enzyme/src/Utils.js

+export function spyMethod(instance, methodName) {
+  let lastReturnValue;
+  const originalMethod = instance[methodName];
+  const hasOwn = Object.hasOwnProperty.call(instance, methodName);


this should use the has package

ljharb · 2017-10-17T16:33:05Z

packages/enzyme/src/Utils.js

+    restore() {
+      if (hasOwn) {
+        /* eslint-disable no-param-reassign */
+        instance[methodName] = originalMethod;


this won't actually restore the original property descriptor; to do this properly, it needs to use Object.getOwnPropertyDescriptor and Object.defineProperty when available, and fall back to normal reading and assignment.

ljharb · 2017-10-17T16:33:28Z

packages/enzyme/src/Utils.js

+        /* eslint-enable no-param-reassign */
+      }
+    },
+    get lastReturnValue() {


I don't think this needs to be, or should be, a getter - can it just be a function?

ljharb

Seems good, thanks

koba04 · 2018-02-12T07:37:29Z

@ljharb

React has plans for v16.3. facebook/react#12152
They include API changes for lifecycle methods.

Add UNSAFE_ prefix lifecycle methods
- UNSAFE_componentWillMount, UNSAFE_compnentWillReceiveProps, UNSAFE_componentWillUpdate
Add static getDerivedStateFromProps

Currently, ShallowWrapper overrides componentWillReceiveProps and shouldComponentUpdate to get the result of shouldComponentUpdate.
It means enzyme have to deal with these changes at ShallowWrapper.

This PR will solve that.
This PR avoid to override these lifecycle methods by using a spy function.
So if this PR is merged, we don't have to care about the changes.

koba04 · 2018-03-30T01:42:08Z

React v16.3 has been released.
The current implementation of shallow doesn't cover the case that UNSAFE_componentWillReceiveProps should be called when shouldComponentUpdate returns false because enzyme overrides the implementation.
This PR will resolve the case.

https://github.com/airbnb/enzyme/pull/1192/files#diff-fa27c82ee4fde075bae6173848e17c82L263

koba04 · 2018-07-03T01:14:16Z

I'm not sure why CI is failing...

https://travis-ci.org/airbnb/enzyme/jobs/399026371

koba04 · 2018-07-04T03:16:11Z

All checks have passed 🎉

ljharb · 2018-07-04T05:14:54Z

packages/enzyme/src/ShallowWrapper.js

            if (
-              !this[OPTIONS].disableLifecycleMethods &&
-              instance
+              lifecycles.getSnapshotBeforeUpdate


this no longer seems to be checking disableLifecycleMethods?

Checking disableLifecycleMethods is here.

https://github.com/airbnb/enzyme/pull/1192/files#diff-fa27c82ee4fde075bae6173848e17c82R338

ljharb · 2018-07-04T05:15:49Z

packages/enzyme/src/ShallowWrapper.js

-                lifecycles.getSnapshotBeforeUpdate
-                && typeof instance.getSnapshotBeforeUpdate === 'function'
-              ) {
-                const snapshot = instance.getSnapshotBeforeUpdate(prevProps, state);


i have a mild preference for splitting the "snapshot" and "context" paths entirely, instead of trying to interleave them with let snapshot

ljharb · 2018-07-04T05:18:11Z

packages/enzyme/src/Utils.js

+  }
+  Object.defineProperty(instance, methodName, {
+    configurable: true,
+    enumerable: false,


shouldn't this be !descriptor || !!descriptor.enumerable?

But shouldn't an instance method be enumerable?

the replacement should be enumerable, or not, to match the one it's spying on

koba04 · 2018-07-04T06:55:42Z

packages/enzyme/src/ShallowWrapper.js

+              if (typeof instance.getSnapshotBeforeUpdate === 'function') {
+                snapshot = instance.getSnapshotBeforeUpdate(prevProps, state);
+              }
+              if (typeof instance.componentDidUpdate === 'function') {


Should I check lifecycles.componentDidUpdate?

yes, that'd be great

ljharb · 2018-07-04T07:02:19Z

packages/enzyme/src/ShallowWrapper.js

          let shouldRender = true;
-          // dirty hack:
-          // make sure that componentWillReceiveProps is called before shouldComponentUpdate


do we no longer need this hack?

Can you elaborate on why?

The current version calls shouldComponentUpdate manually to get the result.
But componentWillReceiveProps should be called before shouldComponentUpdate so we should call componentWillReceiveProps manually before shouldComponentUpdate.

In this PR, we no longer call shouldComponentUpdate manually because we can spy the method.
As the result, we can remove the dirty hack because we don't need to call componentWillReceiveProps and shouldComponentUpdate manually.

Perfect, thanks.

koba04 · 2018-07-05T08:58:05Z

Thanks 🎉

koba04 added 2 commits September 28, 2017 15:00

[New] add spyMethod

9d6e304

[Fix] shallow: Use spyMethod to inspect the result of `shouldComp…

f1cf02b

…onentUpdate`/`getSnapshotBeforeUpdate`

koba04 commented Sep 28, 2017

View reviewed changes

ljharb reviewed Sep 28, 2017

View reviewed changes

ljharb added semver: patch API: shallow labels Sep 28, 2017

ljharb approved these changes Sep 29, 2017

View reviewed changes

ljharb requested a review from a team September 29, 2017 14:20

koba04 changed the title ~~Use sinon.spy to inspect the result of shouldComponentUpdate~~ Implement spyMethod and use it to inspect the result of shouldComponentUpdate Oct 17, 2017

ljharb requested changes Oct 17, 2017

View reviewed changes

ljharb approved these changes Oct 18, 2017

View reviewed changes

ljharb requested review from nfcampos, blainekasten, aweary, a team and lelandrichardson October 18, 2017 06:43

koba04 force-pushed the use-sinon branch from 71be2d4 to 4103c0e Compare October 18, 2017 08:49

koba04 force-pushed the use-sinon branch from 4103c0e to f013d77 Compare November 6, 2017 07:03

koba04 force-pushed the use-sinon branch from f013d77 to 8481710 Compare November 16, 2017 01:14

koba04 force-pushed the use-sinon branch from 8481710 to 127ac0a Compare November 28, 2017 04:29

koba04 mentioned this pull request Dec 30, 2017

Fix to call a setState callback after finishing the the render #1453

Merged

koba04 force-pushed the use-sinon branch from 8a2eae7 to 622c345 Compare February 12, 2018 07:12

koba04 force-pushed the use-sinon branch from 622c345 to 3404c69 Compare March 30, 2018 01:34

koba04 mentioned this pull request Mar 31, 2018

Support getSnapshotBeforeUpdate for shallow #1602

Closed

10 tasks

koba04 force-pushed the use-sinon branch from 3404c69 to fc64f91 Compare May 19, 2018 16:03

koba04 force-pushed the use-sinon branch 2 times, most recently from 8795acb to 50ddc88 Compare July 2, 2018 02:11

koba04 mentioned this pull request Jul 2, 2018

Add getSnapshotBeforeUpdate support into shallow #1657

Merged

koba04 force-pushed the use-sinon branch 2 times, most recently from 312df6b to d5dc160 Compare July 2, 2018 08:12

koba04 mentioned this pull request Jul 4, 2018

componentWillReceiveProps is always invoked in shallow, even getDerivedStateFromProps exists #1692

Closed

3 tasks

koba04 force-pushed the use-sinon branch from d5dc160 to fb769c1 Compare July 4, 2018 02:37

ljharb reviewed Jul 4, 2018

View reviewed changes

ljharb added semver: minor New stuff. package: enzyme-adapter-utils labels Jul 4, 2018

koba04 commented Jul 4, 2018

View reviewed changes

ljharb reviewed Jul 4, 2018

View reviewed changes

koba04 force-pushed the use-sinon branch from ca7b9af to 6fa1e9a Compare July 4, 2018 08:42

ljharb force-pushed the use-sinon branch from 6fa1e9a to f1cf02b Compare July 5, 2018 06:32

ljharb merged commit f1cf02b into enzymejs:master Jul 5, 2018

koba04 deleted the use-sinon branch July 5, 2018 08:58

This was referenced Oct 1, 2018

Update enzyme in /modules/frontend from 3.3.0 to 3.6.0 project-accelerate/accelerate#265

Open

Update enzyme in / from 3.3.0 to 3.6.0 leonardofaria/leozera-ui#234

Closed

Implement spyMethod and use it to inspect the result of shouldComponentUpdate #1192

Implement spyMethod and use it to inspect the result of shouldComponentUpdate #1192

Conversation

koba04 commented Sep 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

ljharb left a comment

Choose a reason for hiding this comment

koba04 commented Sep 29, 2017

lelandrichardson commented Sep 30, 2017

ljharb commented Sep 30, 2017

lelandrichardson commented Sep 30, 2017

lelandrichardson commented Sep 30, 2017

ljharb commented Sep 30, 2017

koba04 commented Oct 14, 2017

koba04 commented Oct 17, 2017

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

koba04 commented Feb 12, 2018

koba04 commented Mar 30, 2018

koba04 commented Jul 3, 2018

koba04 commented Jul 4, 2018

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

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

koba04 commented Jul 5, 2018