Enzyme 3 #198

rylanc · 2017-09-27T01:27:12Z

No description provided.

itsdouges · 2017-09-27T02:37:43Z

you have some linting errors.

ljharb · 2017-09-27T02:57:39Z

package.json

-    "enzyme": "1.x || ^2.3.0",
-    "react": "^0.14.0 || ^15.0.0-0",
-    "react-dom": "^0.14.0 || ^15.0.0-0"
+    "cheerio": "0.19.x || 0.20.x || 0.22.x || 1.0.0-rc.2",


enzyme 3 requires cheerio v1 or higher - this needs to change to ^1.0.0-rc.2

I think #196 has got the dependencies pretty much right

yes as a nonbreaking change #196 does this much better (altho enzyme 2 requires cheerio 0.22 so the 0.19 and 0.20 peer deps aren't helpful)

ljharb · 2017-09-27T02:58:01Z

package.json

@@ -58,13 +58,14 @@
    "babel-register": "^6.5.2",
    "chai": "^3.0.0 || ^4.0.0",
    "cross-env": "3.1.4",
-    "enzyme": "^2.3.0",
+    "enzyme": "3.0.0",


nothing should ever be pinned; these should all use ^.

ljharb · 2017-09-27T02:58:20Z

package.json

-    "react": "^0.14.0 || ^15.0.0-0",
-    "react-addons-test-utils": "^0.14.0 || ^15.0.0-0",
-    "react-dom": "^0.14.0 || ^15.0.0-0",
+    "react": "16.0.0",


dev dep ranges should always exactly match peer dep ranges.

ljharb · 2017-09-27T02:58:44Z

src/ShallowTestWrapper.js

@@ -2,6 +2,8 @@ import $ from 'cheerio'

 import TestWrapper from './TestWrapper'

+const getDisplayName = type => type.displayName || type.name || type


why is this function needed?

ljharb · 2017-09-27T02:58:59Z

src/ShallowTestWrapper.js

+
+    const rootInstance = root.instance()
+    const rootType = rootInstance ? rootInstance.constructor : root.getElement().type
+    const name = rootType ? getDisplayName(rootType) : '???'


root.name() || '???' should suffice here, no?

Enzyme's ShallowWrapper behaves slightly different from the ReactWrapper when it comes to the name() function. Given the following component:

const Example = () => <table />;

mount(<Example />).name() returns 'Example' while shallow(<Example />).name() returns 'table'.

marchaos · 2017-09-27T12:16:47Z

Is there a way that we can get this expedited? With React 16 and enzyme 3 being released last night, this is the final piece for many upgrading.

Thanks!

ayrton · 2017-09-27T18:27:55Z

test/blank.test.js

@@ -1,8 +1,10 @@
 class Fixture extends React.Component {
  render () {
    return (
-      <div id='parent'>
-        <div id='child' />
+      <div>


Why are all these necessary?

Enzyme 3 changes the way it wraps HTML when returning from the render function. The new version returns a cheerio wrapper with a type of tag that IS the parent element (instead of a wrapper with a type root that contains the parent element).

For example:

<div id='parent'> <div id='child' /> <div>

With the above code in Enzyme 3, calling wrapper.find('#parent').length returns 0 while calling wrapper.is('#parent') returns true. With Enzyme 2, wrapper.find('#parent').length returns 1 and wrapper.is('#parent') returns false.

(this is as a result of a breaking change in cheerio v1)

Thanks for explaining, we'll definitely have to document this in our CHANGELOG.md to educate people on how to upgrade

Crazy 3AM idea (jetlag), can we add the extra <div /> in chai-enzyme when using render?

You could wrap it, sure, but then anyone who didn't understand this, and was checking any other method besides .html(), would have the wrong assertions.

In other words, it's better for wrapper.is('#parent') === true and wrapper.html() not to contain the parent div, then for the reverse to be true.

ayrton · 2017-09-27T18:30:10Z

Can you get rid of the yarn.lock file, if we include this we should do it in a separate PR

ljharb · 2017-09-27T18:34:09Z

I'd suggest adding package-lock=false to .npmrc, and yarn.lock, package-lock.json, and npm-shrinkwrap.json to .gitignore :-)

ayrton · 2017-09-27T18:34:25Z

@marchaos we're trying to release a react 16 compatible version as soon as possible. Any help is appreciated :)

jugglinmike · 2017-09-27T18:54:50Z

@jugglinmike, any idea on these cheerio test failures and how to fix them?

@ljharb I just pulled this branch down and ran it, but the tests are passing on my end. It's difficult to see the problem on TravisCI because the jobs are halting on the previously-identified linting error. It doesn't look like any new commits have been push up, so I think I may be doing something wrong here. Should I be using some command other than npm test?

ljharb · 2017-09-27T18:59:18Z

@jugglinmike travis only runs https://github.com/producthunt/chai-enzyme/blob/master/.travis.yml#L12-L14 - that should be sufficient.npm run test:unit by itself should also skip the linter.

jugglinmike · 2017-09-27T20:35:25Z

Still no dice, I'm afraid. The command

$ npm run test:unit

Finishes with:

  431 passing (2s)
$ echo $?
0

For context:

$ node --version
v6.10.3
$ npm ls | grep cheerio
│ ├─┬ [email protected]
$ git log --oneline -n4
1afa0ca Fix peerDependencies
b954dd5 :shirt: Fix linter warnings
174bcef Fix to work with enzyme 3
c985924 Support an array of nodes in `contain`

jugglinmike · 2017-09-27T20:39:10Z

Uhg. @ljharb I somehow confused this branch with gh-196--that's where you actually asked for my input. Sorry for the noise, folks.

itsdouges · 2017-09-27T20:40:51Z

Two pull requests will do that :-). Are the two active pull requests racing against each other? What's going on. On 28 Sep. 2017 6:39 am, "jugglinmike" <[email protected]> wrote: Uhg. @ljharb <https://github.com/ljharb> I somehow confused this branch with gh-196 <#196 where you actually asked for my input. Sorry for the noise, folks. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#198 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGfHne5zQnpZA7y5zRDmt651kWqZHzA5ks5smrJvgaJpZM4PlF69> .

rylanc · 2017-09-27T20:41:56Z

Bah. Sorry, I didn't realize there was an existing PR. @jugglinmike, I'm happy to concede the honor to you. Feel free to use anything in this one that helps.

jugglinmike · 2017-09-27T20:44:04Z

Actually, that one isn't mine. I'm just trying to help with integration issues (though it seems you've solved those here).

rylanc · 2017-09-27T20:47:36Z

Oh OK. Well, @ljharb, @ayrton: should the goal be to maintain backwards compatibility with enzyme 2? If so, I should probably add a test config for both.

ljharb · 2017-09-27T20:53:55Z

@rylanc it's always ideal to avoid breaking changes if possible :-)

rylanc · 2017-09-27T21:49:56Z

In order to satisfy the peerDependencies of react-element-to-jsx-string and add compatibility with React 16, we would have to upgrade react-element-to-jsx-string to version 12.0.0. Unfortunately, in version 6.0.0 of react-element-to-jsx-string, it started requiring an ES2015 environment (or polyfill): https://github.com/algolia/react-element-to-jsx-string/blob/master/CHANGELOG.md#600-2017-01-03. It seems like the options are:

Start requiring an ES2015 environment in chai-enzyme
Use an alternative way of converting elements to strings. It looks like we can leverage enyme's ShallowWrapper.debug to achieve a similar output (probably not identical):

function reactElementToJSXString(node) {
  const Wrapper = () => node
  return shallow(<Wrapper />).debug()
}

I'd lean towards the latter, if it were up to me. Let me know what you think

rylanc · 2017-09-28T18:38:19Z

@ljharb The tests are passing for both enzyme 2 and 3 now if you have time to take another look.

ayrton · 2017-09-28T18:42:49Z

@rylanc I'm ok dropping react-element-to-jsx-string if the alternative is good enough. How would the output look like?

rylanc · 2017-09-28T18:57:42Z

@ayrton It's mostly the same. It looks like react-element-to-jsx-string does some fancy formatting of props (function names, deep serialization of objects, etc.) while enzyme's debug() does not.

ljharb · 2017-09-29T14:57:07Z

@rylanc improving debug in enzyme itself is always possible if it doesn't add too much complexity, fwiw

ayrton · 2017-09-29T14:57:50Z

package.json

-    "react": "^0.14.0 || ^15.0.0-0",
-    "react-dom": "^0.14.0 || ^15.0.0-0"
+    "cheerio": "0.19.x || 0.20.x || 0.22.x || ^1.0.0-0",
+    "enzyme": "^2.9.1 || ^3.0.0",


Out of curiosity, is it possible to support "^2.7.0 || ^3.0.0"?

ayrton · 2017-09-29T15:17:57Z

src/wrap.js

@@ -13,7 +13,7 @@ export default function wrap (el) {
    return new ReactTestWrapper(el)
  }

-  if (el && el._root && el.options) {
+  if (el && el.cheerio && el.options) {


This doesn't need a fallback (to _root for old cheerio versions)?

It looks Cheerio.prototype.cheerio has been around since 0.19.0: https://github.com/cheeriojs/cheerio/blob/0.19.0/lib/cheerio.js#L99

ljharb · 2017-10-02T18:34:58Z

package.json

-    "react": "^0.14.0 || ^15.0.0-0",
-    "react-dom": "^0.14.0 || ^15.0.0-0"
+    "cheerio": "0.19.x || 0.20.x || 0.22.x || ^1.0.0-0",
+    "enzyme": "^2.7.0 || ^3.0.0",


this is semver-major since it's dropping v1, but i don't think it's worth retaining v1 support either.

Good point. Since the current version is still < 1.0, does this make it 0.9.0?

IMO we release a 1.0 when this PR is merged in so I wouldn't worry about it

1.0 is always good :-) if it remained sub-1.0 for some reason then it'd be 0.9.0, yes

rylanc · 2017-10-03T17:40:44Z

@ljharb @ayrton Is there anything still missing from this PR?

VidyullathaKandipati · 2017-10-03T23:21:49Z

src/reactNodeToString.js

+
+function reactElementToJSXString (node) {
+  const Wrapper = () => node
+  return shallow(<Wrapper />).debug()


Hey @rylanc I am not sure, if I can comment on this, but I found this while using your changes. Hence, bring to your notice. We need import React from 'react' before creating a react element here.

eslint-plugin-react should be used to lint for this, fwiw.

(and the airbnb eslint config has it all set up for you ;-) )

rylanc added 3 commits September 26, 2017 17:58

Fix to work with enzyme 3

174bcef

👕 Fix linter warnings

b954dd5

Fix peerDependencies

1afa0ca

ljharb requested changes Sep 27, 2017

View reviewed changes

ljharb mentioned this pull request Sep 27, 2017

chai-enzyme can not work with enzyme 3.0 and react 16.0.0 #199

Open

layershifter mentioned this pull request Sep 27, 2017

chore(package): update to react 16 Semantic-Org/Semantic-UI-React#2131

Merged

2 tasks

ayrton reviewed Sep 27, 2017

View reviewed changes

rylanc added 2 commits September 27, 2017 13:32

Remove yarn.lock

baf4a40

👕 Fix linter warnings

629c0fa

rylanc added 2 commits September 27, 2017 13:50

Fix package.json versions

e443235

Fix typo

f9a6123

rylanc added 4 commits September 27, 2017 14:52

Don't use react-element-to-jsx-string

12e3050

Add backwards compatibility with Enzyme 2

6bbcd87

💚 Fix CI build

f948940

Uninstall enzyme-adapter-react-15 when testing enzyme 2 in CI

31ad72e

Fix build for npm v3.3.6

0bc48dc

ayrton reviewed Sep 29, 2017

View reviewed changes

enzymejs deleted a comment from dschinkel Sep 29, 2017

ayrton reviewed Sep 29, 2017

View reviewed changes

Support enzyme 2.7.0

02f7f3a

ljharb reviewed Oct 2, 2017

View reviewed changes

ayrton mentioned this pull request Oct 2, 2017

Make compatible with enzyme v3 #196

Closed

ockham mentioned this pull request Oct 3, 2017

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

Merged

ayrton merged commit 8920067 into enzymejs:master Oct 3, 2017

VidyullathaKandipati reviewed Oct 3, 2017

View reviewed changes

		@@ -2,6 +2,8 @@ import $ from 'cheerio'

		import TestWrapper from './TestWrapper'

		const getDisplayName = type => type.displayName \|\| type.name \|\| type

Enzyme 3 #198

Enzyme 3 #198

Conversation

rylanc commented Sep 27, 2017

itsdouges commented Sep 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchaos commented Sep 27, 2017 • 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

ayrton commented Sep 27, 2017

ljharb commented Sep 27, 2017

ayrton commented Sep 27, 2017

jugglinmike commented Sep 27, 2017

ljharb commented Sep 27, 2017

jugglinmike commented Sep 27, 2017

jugglinmike commented Sep 27, 2017

itsdouges commented Sep 27, 2017 via email

rylanc commented Sep 27, 2017

jugglinmike commented Sep 27, 2017

rylanc commented Sep 27, 2017

ljharb commented Sep 27, 2017

rylanc commented Sep 27, 2017

rylanc commented Sep 28, 2017

ayrton commented Sep 28, 2017

rylanc commented Sep 28, 2017

ljharb commented Sep 29, 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

ayrton Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rylanc commented Oct 3, 2017

VidyullathaKandipati Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchaos commented Sep 27, 2017 •

edited

Loading

ayrton Oct 2, 2017 •

edited

Loading

VidyullathaKandipati Oct 3, 2017 •

edited

Loading