-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[Compat] support [email protected] #876
Conversation
README.md
Outdated
|
||
```bash | ||
npm i --save-dev react-dom | ||
npm i --save-dev prop-types |
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.
Does prop-types
work with every version of React?
If it does, then let's just depend on it explicitly instead of making it funky. airbnb-prop-types
will likely depend on it very soon anyways, and enzyme will be depending on that soon if it doesn't already :-)
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.
Sounds great to me 👍
src/react-compat.js
Outdated
// eslint-disable-next-line import/no-extraneous-dependencies | ||
PropTypes = require('prop-types'); | ||
// eslint-disable-next-line import/no-extraneous-dependencies | ||
createClass = require('create-react-class'); |
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.
does this package work with React versions prior to 15.5? 15.0-15.4, or 14, or 13?
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.
I think so. Maybe we should get @acdlite involved here.
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.
@acdlite fwiw, if both of these packages happened to work down to react 0.13, we could drop a lot of our crappy conditional requires here :-)
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.
create-class
should work, but I think the signature for prop-types
may have changed recently. I'll check.
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.
Thanks!
src/react-compat.js
Outdated
} else { | ||
// eslint-disable-next-line import/no-extraneous-dependencies | ||
TestUtils = require('react-addons-test-utils'); | ||
} | ||
} catch (e) { | ||
// eslint-disable-next-line no-console | ||
console.error( |
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.
Does this need to be in the try
block? The error logged in catch
only mentions react-addons-test-utils
so it might be confusing if requiring react-dom/test-utils
throws for whatever reason.
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.
The error message should be conditional just like the require is, good catch.
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 error message still needs updating.
I realize this is kinda high priority, but I can't work on it any more. If we need this fixed asap, someone else will have to take it from here. Good luck friends! |
I filed facebook/react#9371 to fix the fact that |
Nice work @kentcdodds ! There's also a small issue with |
note: If a non-collaborator wants to add to this PR, please push some commits up to your fork, and simply paste a link to them here - I'll be happy to cherry-pick them into the PR so we can all get this done together. |
Created a patch (here) with the changes I mentioned above but notice that we seem to still have problems with the new shallow renderer import. 😦 Using It's getting late here now so I'm going to call it a night. Will look more tomorrow. Edit: The problems with 'react-test-renderer/shallow' are related to batching. Two instances of |
package.json
Outdated
@@ -23,7 +23,7 @@ | |||
"react:clean": "rimraf node_modules/react node_modules/react-dom node_modules/react-addons-test-utils", | |||
"react:13": "rimraf node_modules/.bin/npm && npm run react:clean && npm i [email protected] && npm install", | |||
"react:14": "rimraf node_modules/.bin/npm && npm run react:clean && npm i [email protected] [email protected] [email protected] && npm install", | |||
"react:15": "rimraf node_modules/.bin/npm && npm run react:clean && npm i react@15 react-dom@15 react-addons-test-utils@15 && npm install", | |||
"react:15": "rimraf node_modules/.bin/npm && npm run react:clean && npm i react@15 react-dom@15 react-addons-test-utils@15 prop-types@15 create-react-class@15 && npm install", |
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.
shouldn't we be removing react-addons-test-utils from this line?
src/react-compat.js
Outdated
@@ -18,6 +18,8 @@ let childrenToArray; | |||
let renderWithOptions; | |||
let unmountComponentAtNode; | |||
let batchedUpdates; | |||
let PropTypes; | |||
let createClass; |
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.
should we start importing createClass from here in the tests that use it? otherwise we'll have plenty of warnings
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.
yes, all the tests should pull it from here too.
Shallow rendering turns out to be trickier to decouple from ReactDOM than we expected. We didn’t know that people use Perhaps we should reverse the course on that one and revisit it in 16. |
Never mind my previous comment, I misunderstood how Enzyme works. I was reading |
A problem was discovered while testing the new shallow renderer but has since been fixed by @gaearon (facebook/react/pull/9382) and will be re-released with a point update shortly. Consider adding this commit to pull the shallow renderer from the correct place and this commit to accompany Dan's above PR change. |
@bvaughn thanks, I've cherry-picked in those two commits! |
Please bump |
@gaearon the install command installs |
shouldn't we start testing both 15.4 and 15.5, if not on CI at least have the command to do it locally? |
README.md
Outdated
the following npm modules installed if they were not already: | ||
|
||
```bash | ||
npm i --save-dev react-dom create-react-class |
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.
can't we use create-react-class for all versions, like prop-types?
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.
Most likely yes #876 (comment)
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.
There's one subtle difference. If you pair create-react-class with react-dom < 15.5.0 then the callback param of replaceState
won't work.
Otherwise I've tested back to 0.14.0 and I think it's okay.
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.
@bvaughn is that because that's a new feature in v15.5, or would that work with React.createClass
prior to 15.5?
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.
react-dom | create-react-class | works? |
---|---|---|
15.4.2 | N/A | ✓ |
15.4.2 | 15.5+ | × |
15.5+ | 15.5+ | ✓ |
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.
@nfcampos for the tests can't we just import conditionally create-react-class
ourselves? If the main reason for keeping it is for our tests we should restructure it so our testing requirements aren't being leaked to consumers
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.
(its use in ReactWrapperComponent can be replaced with an ES6 class easily)
I've submitted a PR for that. #877
It doesn't depend on this PR, but can I change the target branch to here?
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.
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.
That seems better - making a test/react-compat
file that handles createClass
, and we can import that for tests, and add create-react-class
as a dev dependency.
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.
Script should probably be updated to test both 15.4.2 and 15.5+ yeah.
|
Let's definitely both 15.4 and 15.5 on CI; local testing that's not enforced by CI is useless :-) |
@ljharb ive pushed a commit testing both react 15 versions |
README.md
Outdated
the following npm modules installed if they were not already: | ||
|
||
```bash | ||
npm i --save-dev react-dom create-react-class |
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.
@bvaughn thanks; if that's the case, then we shouldn't use create-react-class
at all prior to react 15.5 :-/
Is this a bug we could file on the create-react-class
repo and get fixed?
src/ShallowWrapper.js
Outdated
batchedUpdates(fn) { | ||
const renderer = this.root.renderer; | ||
if (renderer.unstable_batchedUpdates) { | ||
// React 15.5+ exposes batching on shallow renderer itself |
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.
should this use REACT155
(or other version constants) so that it's only checked there?
src/ShallowWrapper.js
Outdated
@@ -142,6 +142,16 @@ class ShallowWrapper { | |||
this.complexSelector = new ComplexSelector(buildPredicate, findWhereUnwrapped, childrenOfNode); | |||
} | |||
|
|||
batchedUpdates(fn) { |
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.
instead of exposing another prototype method, what do you think about using a standalone function?
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.
And pass root
there?
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.
Addressed below
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.
Yes, exactly - pass root
, or this
src/version.js
Outdated
@@ -4,3 +4,4 @@ export const VERSION = React.version; | |||
export const REACT013 = VERSION.slice(0, 4) === '0.13'; | |||
export const REACT014 = VERSION.slice(0, 4) === '0.14'; | |||
export const REACT15 = VERSION.slice(0, 3) === '15.'; | |||
export const REACT155 = VERSION.slice(0, 4) === '15.5'; |
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.
maybe = REACT15 && VERSION.slice(3, 5) === '5.';
would be more accurate? I don't want this matching 15.50
one day ;-)
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.
well wouldn't your suggestion match 16.5.0
which is more likely than 15.50
? ;)
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.
no, REACT15 &&
ensures it starts with 15.
.
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.
ah sorry didn't notice that. then i agree
@plemarquand I understand—just saying that it might be easier to find deprecation sites while working on the app than from running tests. |
@gaearon In the case of this patch/issue a lot of people like myself fixed all the deprecations inside their running app, but were still getting them when running their tests because enzyme (and/or other testing libraries) were/are emitting them. |
Oh I see. To be honest I’m a little surprised third party components have already updated. Are you using very little of them? |
as mentioned above, getting these to throw and show the full stack trace when running tests is to cause console.error = warning => { throw new Error(warning); }; i set this up in any test framework that i use and the framework will typically catch the thrown error the same way that it would an assertion error, causing a failure and showing the full stack trace |
@gaearon Yeah I only ran in to it with enzyme, and only for a day before this patch was merged. Most if not all of the major libraries have updated by now. Serves me right for being on the bleeding edge, typically I like to wait a week or two before upgrading but it was a slow day at work... |
Enzyme recently made a fix for console warning if we use React v15.5+ , See issue here: enzymejs/enzyme#876 To fix that, we have to install react-test-render instead of react-addons-test-utils Warning: ``` console.error node_modules/fbjs/lib/warning.js:36 Warning: ReactTestUtils has been moved to react-dom/test-utils. Update references to remove this warning. console.error node_modules/fbjs/lib/warning.js:36 Warning: Shallow renderer has been moved to react-test-renderer/shallow. Update references to remove this warning. ```
* install react-test-render for removing enzyme warning Enzyme recently made a fix for console warning if we use React v15.5+ , See issue here: enzymejs/enzyme#876 To fix that, we have to install react-test-render instead of react-addons-test-utils Warning: ``` console.error node_modules/fbjs/lib/warning.js:36 Warning: ReactTestUtils has been moved to react-dom/test-utils. Update references to remove this warning. console.error node_modules/fbjs/lib/warning.js:36 Warning: Shallow renderer has been moved to react-test-renderer/shallow. Update references to remove this warning. ``` * Update TutorialReact.md
Is this merged in enzyme v2.8.2 ? I still get some warnings :
|
Hi @kopax, enzyme folks, it may not be a bad idea to add a final comment explaining this and locking this PR. In any case, I'll just unsubscribe from updates. |
@kentcdodds do you have an idea on how I could trace this error ? I am trying to find which library is creating this warning. I am not using a lot but all the maintainers says they have corrected it. Would be just nice to know how to produce a more verbose message. I have tried to set |
I would try going to where that warning log is ( |
@kopax You could add something like this to the header of your test: const decorated = console.error;
console.error = message => decorated(new Error(message).stack); |
Where the line 6:15 of that file is I have checked for all of my errors and it all comes from lines where I do In my package.json, I am using enzyme Looking at this merge, I should not need Apparently I still need it and if I remove it I have real error coming from my tests.
|
* install react-test-render for removing enzyme warning Enzyme recently made a fix for console warning if we use React v15.5+ , See issue here: enzymejs/enzyme#876 To fix that, we have to install react-test-render instead of react-addons-test-utils Warning: ``` console.error node_modules/fbjs/lib/warning.js:36 Warning: ReactTestUtils has been moved to react-dom/test-utils. Update references to remove this warning. console.error node_modules/fbjs/lib/warning.js:36 Warning: Shallow renderer has been moved to react-test-renderer/shallow. Update references to remove this warning. ``` * Update TutorialReact.md
I'm getting this error too. React 15.5. |
@peterheard01 For me it was resolved by adding react-test-renderer as a dependency. Source #875 |
* Upgrade XO * Upgrade style-loader and yargs * Upgrade eslint-config-xo-react * Upgrade jest and eslint-plugin-react * Bulk dep upgrade * Bulk package dep upgrade * Fix omit call for lodash@4 * Fix resolveFrom call * Fix loaderUtils usage * Migrate to prop-types and create-react-class packages Closes #338 * Add react-test-renderer dep enzymejs/enzyme#876 * Remove react/react-dom peerDependencies There are two cases of extraneous-dependencies imported: - react/react-dom, which is expected to be present in the user’s codebase - enzyme and other test utils used inside packages, which rely on the root node_modules * Use yarn command consistently throughout docs
Could it be that the @kopax says he gets an error like I think it comes from this Because I have installed |
@donaldpipowitch please file a new issue; this one's pretty long already and posting here pings a lot of people. It's certainly possible tho that webpack detects the |
Okay, done: #968 👍 |
A few deprecation warnings were added in React 15.5. This supports the new APIs that React recommends.
Closes #875