-
-
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
2.8.1 fails with "Cannot find module 'react-test-renderer/shallow'" #892
Comments
Read the README. :P |
This is a breaking change, it shouldn't be shipped in a patch release! |
@pscanf If you're on React 15.4, it doesn't change anything. If you're on React 15.5, it's a bugfix, because React 15.5 had a breaking change. |
The problem is, if your package.json lists react: "^15.4.0" and enzyme: "^2.8.0", then yesterday your module was passing tests on travis, and today it is not, because you have upgraded to the new react and the new enzyme without actually doing anything.
|
Yesterday your module wasn't passing tests because it was upgraded to React 15.5 (due to the semver range), and the new console warnings were failing your tests (assuming you were being a responsible developer and failing your tests on propType warnings). |
I do, in fact, fail on proptype warnings, but not on other warnings. My tests were passing fine. :P Maybe we could change this bit to something like:
? |
Then there'd be no way to warn users that they're missing a required dependency for React 15.5+. |
Well, there is, because they'll still get the warning that react displays
when the module is required. Or you could even print your own warning
there, in the catch block, to console.error. And it's technically not a "required" dependency
until react 16.
|
Being on
I don't agree, react prints warning specifically not to break things, so enzyme shouldn't break things because anyway react would print warnings. It might not a big deal I guess (at least for me, since I was updating dependencies anyway), but I would see it as a breaking change nonetheless (which can cause a bit of frustration, sorry for the slightly stormy comment above). |
Just adding react-test-renderer dependency fixes everything for me. |
@tomjal's fix worked for me. |
@pscanf warnings do break things; that's the flawed logic that leads to adding new warnings in a semver-minor. |
As much as I love this fix, it's not scalable across the millions of monthly installs enzyme and react get. Fixes should follow semver conventions. |
See this discussion about the enzyme change that caused tests to fail: enzymejs/enzyme#892
@lxe they did. React 15.5 shipped a change that was subjectively either breaking or minor, which broke some subset of users and libs. enzyme had two choices here: ship a patch that fixes things for those that React 15.5 broke (causing breakage for those users who weren't broken with React 15.5); or ship a semver-major and leave users on the current major line screwed, and forced to adapt to the React breakage (and thus accommodating those users who weren't broken with React 15.5). semver does not strictly dictate whether "console warnings" are semver-major or not, as such, considering them breaking or non-breaking are both a valid choice. |
Without taking sides, I'll say that the semver folks would probably be happy to discuss any ambiguities in a GitHub issue and update their guidelines if necessary. |
@openjck the issue is on what constitutes "API" - the document says "For this system to work, you first need to declare a public API. This may consist of documentation or be enforced by the code itself." however I claim that the API consists of "anything that's programmatically observable". Additionally, that document does talk about deprecations being in documentation; React adds deprecations in a semver-minor observably in the code, which I believe constitutes an API change. |
React 15.5 shipped a change that was objectively a minor change. It adds a console.log, which isn't even present in production builds. It affects absolutely nothing for my users. Enzyme had a third choice; do exactly the fix I proposed above. With that fix, if you had 15.5 but hadn't gotten around to doing all the 15.5 fixes and things were working, Enzyme would have had a minor patch and things would have kept working. If you had 15.5 and you were a "responsible developer" whose tests break for any console logs, then when Enzyme referenced I'm not sure why a major version would leave anyone "screwed" - it would have let people know they needed to do something when |
I'm using react I'm not so much up to date with news in react, but imo if external libs, depending on enzyme, work with enzyme I just hope I'll be able to use enzyme in future versions, b/c I won't be able to update my react any time soon unfortunately. |
Just to re-iterate from @tiriana's point, we are also using React |
0.14.8 works for me. If you open a new folder and try:
Does that print "pass" or "fail"? Is it possible you have some dependency that's pulling in react 0.15.5 and causing this? You can |
So that prints pass, but when running the unit tests through karma, I get this:
|
If it's affecting users without React 15.5 in their trees, then it is definitely a bug that needs fixing, but I'm not sure how it could be. |
That is definitely a mystery. This is a long shot but, @shahzainb do you have any projects The only place enzyme tries to require 'react-test-renderer' is here, and it's guarded by the |
The enzyme 2.8.1 upgrade also mysteriously broke builds for me as well (we are running react 15.3.2). I am pinning to 2.8.0 |
v2.8.2 published. Please comment if you're still having trouble. |
@jwalton About the snippet you pasted - it logged |
@ljharb Thx for quick reaction. But unfortunately I still get this issue: |
@tiriana can you file a new issue, and include your installed versions of react (and react-related things)? |
@jwalton I actually have react 0.14.9, but nothing seems to pull react 15.5
|
@ljharb sure thing, will do. |
Sorry, but 2.8.2 still fails
|
Still failing my build too, with react 15.3.2 |
if I add |
I'm trying to find where |
See the readme; you need to explicitly add it. |
oh :) sorry |
Upgrading enzyme from 2.8.0 to 2.8.1 or 2.8.2 without upgrading React breaks my build, too. The error is I'm using react 15.4.x. and react-dom 15.4.x Ok, I figured out, you need react-dom 15.5.x for react-dom/test-utils |
@paibamboo it shouldn't attempt to require |
Why is react-test-renderer not declared as as peerDependency? I understand it's only required for [email protected], but I don't see the problem with declaring it. |
Declaring it forces everyone to install it, even if they're not using v15.5. |
2.8.2 still fails for me where 2.8.0 worked. I'm using react, react-dom, and react-addons-test-utils all versioned at 15.4.2; karma 1.7.0, and webpack 2.1. The way I finally made this work was to extend my webpack externals object to be the following:
I tried a few different permutations with react-test-renderer before settling on this solution:
So clearly listing the package as an external for webpack was sufficient. All of my shallow rendering tests still pass, and even adding a failing expectation such as |
@ZebraFlesh solution worked for me |
Sounds like we may need to update the webpack documentation, and then we can close this issue? |
Downgrading to 2.8.0 fixes it.
The text was updated successfully, but these errors were encountered: