-
-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enzyme 3 #198
Enzyme 3 #198
Changes from all commits
174bcef
b954dd5
1afa0ca
baf4a40
629c0fa
e443235
f9a6123
12e3050
6bbcd87
f948940
31ad72e
0bc48dc
02f7f3a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,11 @@ node_js: | |
env: | ||
- CHAI_VERSION=^3.0.0 | ||
- CHAI_VERSION=^4.0.0 | ||
- ENZYME_VERSION=^2 | ||
- ENZYME_VERSION=^3 | ||
before_script: | ||
- npm install chai@$CHAI_VERSION | ||
- if [ "$ENZYME_VERSION" = "^2" ]; then npm uninstall enzyme-adapter-react-15; npm install [email protected]; fi | ||
- npm install enzyme@$ENZYME_VERSION | ||
script: | ||
- npm test |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
import wrap from './wrap' | ||
|
||
export default class ChaiWrapper { | ||
|
||
/** | ||
* Constructs a instance of the chai wrapper. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ import $ from 'cheerio' | |
|
||
import TestWrapper from './TestWrapper' | ||
|
||
const getDisplayName = type => type.displayName || type.name || type | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this function needed? |
||
|
||
export default class ShallowTestWrapper extends TestWrapper { | ||
constructor (wrapper) { | ||
super() | ||
|
@@ -17,11 +19,13 @@ export default class ShallowTestWrapper extends TestWrapper { | |
} | ||
|
||
inspect () { | ||
const name = this.wrapper.root.unrendered.type.displayName || | ||
this.wrapper.root.unrendered.type.name || | ||
'???' | ||
const root = this.root() | ||
|
||
const rootInstance = root.instance() | ||
const rootType = rootInstance && rootInstance.constructor | ||
const name = rootType ? getDisplayName(rootType) : (root.name() || '???') | ||
|
||
if (this.wrapper.root === this.wrapper) { | ||
if (root === this.wrapper) { | ||
return `<${name} />` | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,9 @@ | ||
import reactElementToJSXString from 'react-element-to-jsx-string' | ||
import { shallow } from 'enzyme' | ||
|
||
function reactElementToJSXString (node) { | ||
const Wrapper = () => node | ||
return shallow(<Wrapper />).debug() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. eslint-plugin-react should be used to lint for this, fwiw. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (and the airbnb eslint config has it all set up for you ;-) ) |
||
} | ||
|
||
function reactArrayToJSXString (nodes) { | ||
let jsxString = '[' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't need a fallback (to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks |
||
return new CheerioTestWrapper(el) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,10 @@ | ||
class Fixture extends React.Component { | ||
render () { | ||
return ( | ||
<div id='parent'> | ||
<div id='child' /> | ||
<div> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are all these necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enzyme 3 changes the way it wraps HTML when returning from the For example: <div id='parent'>
<div id='child' />
<div> With the above code in Enzyme 3, calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (this is as a result of a breaking change in cheerio v1) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for explaining, we'll definitely have to document this in our There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Crazy 3AM idea (jetlag), can we add the extra There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could wrap it, sure, but then anyone who didn't understand this, and was checking any other method besides In other words, it's better for |
||
<div id='parent'> | ||
<div id='child' /> | ||
</div> | ||
</div> | ||
) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is semver-major since it's dropping v1, but i don't think it's worth retaining v1 support either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Since the current version is still < 1.0, does this make it 0.9.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we release a 1.0 when this PR is merged in so I wouldn't worry about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.0 is always good :-) if it remained sub-1.0 for some reason then it'd be
0.9.0
, yes