Enzyme: [v3] Consider adding react-test-renderer to the deps for ReactSixteenAdapter

[mattkrick]

Moving to v3, on React v16, I get the following error:

 Cannot find module 'react-test-renderer/shallow' from 'ReactSixteenAdapter.js'

I think it'd be nice to explicitly depend on this so folks don't need to install it on their own.

[lelandrichardson]

Like react and react-dom, this is a peer dependency: https://github.com/airbnb/enzyme/blob/master/packages/enzyme-adapter-react-16/package.json#L44

We could make it an explicit dep, but then I think we'd have to also add react as a dep? I'm concerned about this causing other errors from people having duplicate react versions in their node_modules.

[ljharb]

@mattkrick what does npm ls output? If it exits nonzero, then nothing can be expected to work.

Every React thing must always be a peer dep, and always must be explicitly depended on in the top level - that's how singletons work.

[lelandrichardson]

@ljharb react and react-test-renderer are both peer deps already. i'm saying i think if we moved react-test-renderer to a dep, then react would need to move to be a dep as well? i'm not quite sure. Anyway, I feel like react-test-renderer should stay a peer dep unless anyone has a convincing argument for it not to be?

[ljharb]

Absolutely it should; react things should never be anything but peer deps except in the top-level app.

[mattkrick]

i agree with keeping react & react-dom as a peerDep.
With React 16, facebook has removed all "add-ons" from react (prop-types, react-transition-group, react-test-renderer, etc). the dependencies for react-test-renderer are:

"dependencies": {
    "fbjs": "^0.8.16",
    "object-assign": "^4.1.1"
  },
  "peerDependencies": {
    "react": "^16.0.0-beta.5"
  },

since everyone will need this to use enzyme, _and it doesn't have a hard dep on react_, i think it's a good candidate for a dependency in enzyme.

[ljharb]

Absolutely not; since enzyme is used in an app, having enzyme-adapter-react-16 hard-depend on react would risk duplicates occuring.

NOTHING EVER can safely directly depend on react except the top-level app, period.

[mattkrick]

@ljharb Shoot, I could've explained myself a little more clearly. react-test-renderer is _not_ react. It's its own distinct package that only has a peerDependency to react:
https://yarnpkg.com/en/package/react-test-renderer

I'm asking to include the package above in the deps.

• react-test-renderer :+1:
• react :-1:

[ljharb]

That seems more reasonable, but the convention is to make it a peer dep.

[mattkrick]

perfect! Now that we got the premise set, lemme argue for promoting it from a peerDep to a a dep.

• a peer dep assumes that the application would have the "host" already installed. React is a perfect example: one can safely assume that if I'm using enzyme, I'm already using react.
• the opposite applies to react-test-renderer: if I'm using enzyme, I probably _don't_ have react-test-renderer installed. That's the beauty of enzyme!
• consider lodash: why is lodash a hard dependency for ReactSixteenAdapter but react-test-renderer is a peer dep? Both are needed for enzyme to run, neither are needed for my application to run.
• at the very least, it should be documented that folks need to additionally install react-test-renderer for v16, but extra installation steps are bummers.

[ljharb]

Conceptually, everything should be a dep, already-installed or not - a peerDep is required when something has state such that it can't be duplicated in the tree.

I certainly agree that if it's a peer dep, it's worth documenting it, but npm ls tells you that you need it just fine.

[mattkrick]

that's a solid definition!
so state == peerDep, stateless = dep?
for my own edification, could you show me how you determined that react-test-renderer keeps state?

[ljharb]

I haven't determined that :-) but historically, all of the react tram's packages that are prefixed with "react" need to be peer deps, and need to have matching major and minor versions, or else everything breaks. (Note that "relies on instanceof" counts as "state" too)

[bdwain]

@ljharb As someone who is trying to upgrade to v3 right now, I can say this tripped me up as well.

Also, the current strategy of just a peer dependency doesn't ensure the exact same version is installed anyway. That relies on the user of the package installing the same versions of them both, but they could easily upgrade one without the other, especially because users are generally not thinking about react-test-renderer since they don't use it directly. If i just said yarn upgrade react, that would not upgrade react-test-renderer, and they would be out of sync.

If react-test-renderer x.y.z truly relies on react x.y.z only, its peer dependency on react should not use a wild card and instead be hard coded to x.y.z. And really it should just have an explicit dependency on that version, not peer, because if it's a different version, it'll break anyway. That seems like it wouldn't be the case though. I can't find any documentation mentioning that.

I think the distinction between regular dependencies and peer dependencies is that for peer dependencies, the end user is expected to need to use them explicitly. Enzyme can't dictate the react version that I use in my app because I need to dictate it as someone who uses it explicitly. Enzyme can rely on me setting the version of react because I would never install enzyme without also installing react.

React-test-renderer is completely different. I've been using enzyme for over a year and I still have no clue what it does because I don't use it directly. I just installed it because enzyme errors without it. I would imagine that most enzyme users have done the same thing, because in a typical use of enzyme, you don't need to use react-test-renderer directly.

Asking users to install something they don't use is always going to be a source of confusion. This is basically the same issue as #940 and #1061, because It was the same problem in enzyme v2.

If there is truly no way around this, then I think it should be very clearly documented in the installation instructions right next to npm install enzyme (Most people don't generally run npm ls to verify all of their dependencies). But that seems very unlikely to me. I've never had to install a package before that I wasn't using explicitly. I think it would be safe for enzyme to make react-test-renderer a regular dependency, though it may require react setting up stricter dependency rules for it with react.

EDIT: I forgot to say. The only reason I've heard for why different versions of things cause problems is because of the scenario where my app depends on react version foo, but a library depends on version bar of react, and so i get 2 copies and each uses a different implementation. I don't think it has anything to do with the version of test renderer needing to be the same as the version of react. Otherwise they'd document that/specify it in package.json. And this would not be a problem in this scenario because react-test-renderer only has a peer dependency on react, so it will never install its own copy anyway.

[ljharb]

The issue is if two copies of react-test-renderer will break things. If not, it can be a dep; if so, it must be a peer dep. This is true throughout the npm ecosystem.

If you don't run npm ls to verify things, then no matter how uncommon it may be, you're risking breakage. Similarly, if yarn is failing to properly update or notify you about missing peer deps, that's a bug in yarn that should probably be reported.

If someone wants to look into it and determine if react-test-renderer is safe as a dep, then of course we'd move it - peer deps are annoying for users, even tho they're the only solution to this.

[bdwain]

The issue is if two copies of react-test-renderer will break things

This is only true if it made sense for someone to use something that depended on react-test-renderer alongside enzyme. Since enzyme is doing the rendering, there's not really a reason to pull in another renderer. At least not one that would run at the same time as enzyme.

Regardless, I can try to find out if it would be safe to move to a dependency.

[bdwain]

It should be fine to be a dependency. Since react test renderer has its own peer dependency, they are implying that any of those versions are fine to use with react test renderer. I've asked around a bunch in the react channel on reactiflux and no one seems to think it would be unsafe.

[ljharb]

Fixed in #1252 and #1234