ensure ShallowWrapper render output can't get stale #490

jwbay · 2016-07-08T05:11:54Z

Replace .node and .nodes with 'get' functions in ShallowWrapper to ensure render output is always fresh. Add equivalent functions in ReactWrapper. Removes need for ShallowWrapper's .update, fixes #360

Many thanks to @guncha for the idea in #360.

ljharb · 2016-07-08T05:24:38Z

src/ShallowWrapper.js

-      this.node = this.renderer.getRenderOutput();
-      this.nodes = [this.node];
+      Object.defineProperty(this, 'node', {
+        get() {


ugh :-/ getters are incredibly slow and deoptimize all code they touch. I'd rather do a breaking change to make this a method, than use a getter.

I would also be down for using functions. I've always felt nervous about using .node / .nodes as I felt like it was reaching into enzyme internals that could be changed without notice.. Functions feel more like a public API.

Well, node and nodes aren't technically public API are they? We don't document them as far as I can see. I agree that using a function would be an improvement, so maybe we can make that change for the next breaking release and then add node() and nodes() to the API documentation?

Everything accessible - not just everything documented - is public API, even if it has a magical underscore in the front of the property name.

So any change to any object property would be considered a breaking change? If someone is using ReactWrapper.complexSelector.find and we rename/move that, would that require a major release? Maybe we're not talking about the same thing. I usually define public API as the API that we document for the public and that we support.

Getters and setters are horrifically bad for readability and maintainability. imo they should never be used, in any project. A property should be a property, not a function.

Since this is not a bug fix, there's no urgency to add this change, and I'd prefer to convert .node to a function if we want more behavior than "data property".

While I agree that getters don't improve readability, this is definitely a bug. Ask anyone with passing familiarity with Enzyme about what this should do and compare it with what actually happens:

let wrapper = Enzyme.shallow(<MyInput debounce={ 500 } />) wrapper.simulate("keypress", {char: "b"}) wrapper.simulate("keypress", {char: "a"}) wrapper.simulate("keypress", {char: "r"}) expect(wrapper.find(".result").text()).toBe("") jasmine.clock().tick(1000) expect(wrapper.find(".result").text()).toBe("bar")

The requirement to manually update() is intentional. I agree it's a UX improvement to do it automatically, but it's not a bug.

@ljharb I can convert these to functions. Would that be acceptable? Want to make sure because it's a more involved change. 😄 I understand it would have to wait for a 3.x release.

Are there any concerns about ReactWrapper having properties for these and ShallowWrapper having functions?

I think we should convert both to functions at the same time, for consistency.

jwbay · 2016-07-10T04:15:17Z

This is ready for a second look. The first commit was only updated to get tests passing on older versions of React/Node.

I didn't switch everything in ReactWrapper to use the new functions because the only benefit would've been for internal uniformity at the cost of function calls where they weren't needed. Can switch if desired.

Outstanding questions:

Should the new functions should be added to documentation?
How should ShallowWrapper's .update be handled?
- soft deprecation (current approach), possibly with a logged warning
- outright removed since this is already a breaking change
- repurposed to call .instance().forceUpdate() for non-SFCs

ljharb · 2016-07-10T16:38:29Z

docs/api/shallow.md

@@ -169,9 +169,6 @@ Manually sets context of the root component.
 #### [`.instance() => ReactComponent`](ShallowWrapper/instance.md)
 Returns the instance of the root component.

-#### [`.update() => ShallowWrapper`](ShallowWrapper/update.md)


This method still seems useful in case someone mutates state/props without calling the appropriate methods and wants to force an update.

ljharb · 2016-07-10T16:39:08Z

fwiw I think internal uniformity is much more important than avoiding function calls, which are pretty cheap anyways.

jwbay · 2016-09-17T00:26:53Z

Does anyone have further thoughts on this?

ljharb · 2016-09-17T03:09:16Z

The more I think about this, the more I'm confused why there's both "getNode" and "getNodes"?

jwbay · 2016-09-17T03:46:16Z

Don't think there's a reason besides avoiding lots of [0] when you're dealing with one node, but I'm not familiar with the reasoning for the original split between .node and .nodes

ljharb · 2016-09-17T03:49:59Z

If it's just a convenience, perhaps getNode should throw when there's more than one? Also I don't think we need two internal sources of truth - this.nodes seems sufficient.

jwbay · 2016-09-17T04:44:18Z

Good points, updated!

lelandrichardson · 2016-09-24T08:20:26Z

@jwbay this is a great and very welcome change!

Although I agree w/ @ljharb that with a .getNode() method, keeping track of .node and .nodes is potentially unnecessary... however, i'm worried that if we get rid of .node, then we might need to consider this a breaking change. Although .node isn't really an advertised property of the wrapper object, I know that in practice people use it.

Thoughts?

jwbay · 2016-09-24T15:46:26Z

@lelandrichardson Thanks! I agree this is a breaking change.

ljharb · 2016-09-24T16:14:37Z

I'd also be fine keeping the internal .node updated for now, but removing it in the next major version bump.

jwbay · 2016-09-24T16:20:41Z

So restore the old .update functionality? Should it still call .forceUpdate on the instance as well? It didn't before this PR.

ljharb · 2016-09-24T16:23:06Z

@jwbay ah, if the PR can't be easily made to be semver-minor, then we may as well go full semver-major :-)

jwbay · 2016-09-24T17:18:52Z

Oh I don't mind making the change, especially if it can get this shipped sooner :) I just wasn't sure whether adding .forceUpdate calls where there weren't any before would be a breaking change as well. Seems like it would actually. I'll take a look at what's involved for back-compat.

jwbay · 2016-09-28T01:53:21Z

@ljharb @lelandrichardson we should be semver-minor now 👍

lencioni · 2016-10-12T15:19:09Z

LGTM

jwbay · 2016-10-19T04:32:40Z

Resolved conflicts.

bookman25 · 2016-10-23T23:39:53Z

Is there any chance of this branch getting merged? This would reduce a lot of boilerplate code for our team.

…tput is always fresh. removes need for .update

kentor · 2016-11-09T17:29:19Z

hi! any timeline on releasing this?

ljharb · 2016-11-09T19:52:09Z

I'll cut a few releases after #670 gets in.

ChrisFindlay · 2016-12-20T21:51:25Z

Seems to work under jest but not under jest --coverage...

2.5.0 supports passing callbacks to `setState`, which we require for `client/auth/test/login`. Furthermore, there's a fix in 2.6.0 that's required for this to actually work (enzymejs/enzyme#490) Not updating to >=3.0.0 yet since chai-expect isn't compatible (yet). Changelog: https://github.com/airbnb/enzyme/blob/master/CHANGELOG.md#252-november-9-2016

…8472) * Auth: Prettify login.jsx * Auth: Run i18n codemod on login.jsx * Auth: Run react-create-class codemod on login.jsx * Auth: Prettify login.jsx * Auth: Simplify login.jsx * Auth: Named export for un-localized Login component * Framework: Upgrade enzyme to 2.9.1 2.5.0 supports passing callbacks to `setState`, which we require for `client/auth/test/login`. Furthermore, there's a fix in 2.6.0 that's required for this to actually work (enzymejs/enzyme#490) Not updating to >=3.0.0 yet since chai-expect isn't compatible (yet). Changelog: https://github.com/airbnb/enzyme/blob/master/CHANGELOG.md#252-november-9-2016 * Auth: Simplify FormTextInput ref (focus) * Auth: Use enzyme to test login.jsx * Auth: Remove obsolete refs from login.jsx (These were only used by tests.) * Auth: Move SelfHostedInstructions and LostPassword to separate files

ljharb reviewed Jul 8, 2016
View reviewed changes

guncha added a commit to guncha/enzyme that referenced this pull request Jul 8, 2016

Add a benchmark for changes in enzymejs#490

633b0c5

jwbay force-pushed the update branch 5 times, most recently from 67699b6 to d27d5e4 Compare July 10, 2016 00:30

jwbay changed the title ~~use getter for .node and .nodes in ShallowWrapper, deprecate .update~~ ensure ShallowWrapper render output can't get stale Jul 10, 2016

ljharb reviewed Jul 10, 2016
View reviewed changes

jwbay force-pushed the update branch from 576f975 to 51e307a Compare July 20, 2016 23:28

jwbay force-pushed the update branch from 51e307a to 79ac4a7 Compare September 16, 2016 23:25

ljharb added API: shallow semver: major Breaking changes labels Sep 24, 2016

jwbay force-pushed the update branch from 663a552 to cf85072 Compare September 28, 2016 01:50

ljharb added API: mount semver: minor New stuff. labels Sep 28, 2016

jwbay force-pushed the update branch 2 times, most recently from fb76502 to 20408fd Compare October 19, 2016 04:19

jwbay added 10 commits October 23, 2016 22:28

use getter for .node and .nodes in ShallowWrapper to ensure render ou…

8e9ace4

…tput is always fresh. removes need for .update

add .getNode and .getNodes, deprecate .update

d6a51b7

repurpose .update() to handle impure renders and props/state mutation

74f62b6

align ReactWrapper's handling of nodes to match ShallowWrapper's

db3d717

account for react 0.13 and node <= 12

d6f432c

lint fixes

9172828

don't need to track initialNode separately

bcf175c

test new getNode functions

5014baa

restore back-compat behavior for wrapper.node(s)

4cf36f2

fix lint error

18bb997

jwbay force-pushed the update branch from 20408fd to 18bb997 Compare October 24, 2016 02:28

lencioni merged commit 47b4436 into enzymejs:master Oct 24, 2016

jwbay deleted the update branch October 24, 2016 22:28

This was referenced Dec 20, 2016

Problems with ReactDOM.findDOMNode() #479

Closed

Add missing docs for .getNode and .getNodes #743

Closed

This was referenced Sep 28, 2017

auto update wrappers behind a flag #1186

Closed

Restore wrapper auto-update behavior behind a feature flag? #1175

Open

ockham mentioned this pull request Oct 3, 2017

Auth: Run codemods; use enzyme, remove mixin injection from tests Automattic/wp-calypso#18472

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ensure ShallowWrapper render output can't get stale #490

ensure ShallowWrapper render output can't get stale #490

jwbay commented Jul 8, 2016 •

edited

Loading

ljharb Jul 8, 2016

blainekasten Jul 8, 2016 •

edited

Loading

aweary Jul 8, 2016

ljharb Jul 8, 2016

aweary Jul 8, 2016

ljharb Jul 8, 2016

guncha Jul 8, 2016

ljharb Jul 8, 2016

jwbay Jul 8, 2016

ljharb Jul 9, 2016

jwbay commented Jul 10, 2016

ljharb Jul 10, 2016

ljharb commented Jul 10, 2016

jwbay commented Sep 17, 2016

ljharb commented Sep 17, 2016

jwbay commented Sep 17, 2016 •

edited

Loading

ljharb commented Sep 17, 2016

jwbay commented Sep 17, 2016

lelandrichardson commented Sep 24, 2016

jwbay commented Sep 24, 2016

ljharb commented Sep 24, 2016

jwbay commented Sep 24, 2016

ljharb commented Sep 24, 2016

jwbay commented Sep 24, 2016

jwbay commented Sep 28, 2016

lencioni commented Oct 12, 2016

jwbay commented Oct 19, 2016

bookman25 commented Oct 23, 2016

kentor commented Nov 9, 2016 •

edited

Loading

ljharb commented Nov 9, 2016

ChrisFindlay commented Dec 20, 2016

ensure ShallowWrapper render output can't get stale #490

ensure ShallowWrapper render output can't get stale #490

Conversation

jwbay commented Jul 8, 2016 • edited Loading

Choose a reason for hiding this comment

blainekasten Jul 8, 2016 • edited Loading

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

jwbay commented Jul 10, 2016

Choose a reason for hiding this comment

ljharb commented Jul 10, 2016

jwbay commented Sep 17, 2016

ljharb commented Sep 17, 2016

jwbay commented Sep 17, 2016 • edited Loading

ljharb commented Sep 17, 2016

jwbay commented Sep 17, 2016

lelandrichardson commented Sep 24, 2016

jwbay commented Sep 24, 2016

ljharb commented Sep 24, 2016

jwbay commented Sep 24, 2016

ljharb commented Sep 24, 2016

jwbay commented Sep 24, 2016

jwbay commented Sep 28, 2016

lencioni commented Oct 12, 2016

jwbay commented Oct 19, 2016

bookman25 commented Oct 23, 2016

kentor commented Nov 9, 2016 • edited Loading

ljharb commented Nov 9, 2016

ChrisFindlay commented Dec 20, 2016

jwbay commented Jul 8, 2016 •

edited

Loading

blainekasten Jul 8, 2016 •

edited

Loading

jwbay commented Sep 17, 2016 •

edited

Loading

kentor commented Nov 9, 2016 •

edited

Loading