Skip to content
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

Bug: react-test-renderer@17.0.0 incorrectly peer-depends on react@^16.0.0 #20063

Closed
billyjanitsch opened this issue Oct 20, 2020 · 8 comments
Closed
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@billyjanitsch
Copy link
Contributor

React version: react-test-renderer@17.0.0

Steps To Reproduce

  1. Run npm view react-test-renderer@17.0.0 dependencies.
  2. Note the dependency on react-shallow-renderer@^16.13.1.
  3. Run npm view react-shallow-renderer@16.13.1 peerDependencies.
  4. Note the peer dependency on react@^16.0.0.

Alternatively, in an empty project:

  1. Run npm install react@17 react-test-renderer@17.
  2. Note the invalid peer dependency warning.

The current behavior

It doesn't make sense for react-test-renderer@17 to transitively peer-depend on react@16. I assume this was an oversight in the release.

The expected behavior

One should be able to install react@17 and react-test-renderer@17 and have the peer dependencies line up.

@billyjanitsch billyjanitsch added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Oct 20, 2020
@gaearon
Copy link
Collaborator

gaearon commented Oct 20, 2020

Yeah there was some awkwardness here because react-shallow-renderer also depends on react-is. I think we need to make a release of react-shallow-renderer first which bumps to 17 and then do another patch of 17.

@billyjanitsch
Copy link
Contributor Author

billyjanitsch commented Oct 20, 2020

It actually appears to be a mistake for react-test-renderer to depend on react-shallow-renderer at all. react-test-renderer@16.14.0 has no dependency on react-shallow-renderer, and there are no references to it in the source.

The dependency was added in #18348 but maybe that was an oversight?

@gaearon
Copy link
Collaborator

gaearon commented Oct 20, 2020

No, it definitely uses it: https://unpkg.com/browse/react-test-renderer@17.0.0/shallow.js

You mean to look at 16.14, not 16.4?

@billyjanitsch
Copy link
Contributor Author

billyjanitsch commented Oct 20, 2020

Oh, my bad, I was looking in the src/ directory only.

(And yeah, "16.4" was a typo; I meant "16.14". See the output of npm view react-test-renderer@16.14.0 dependencies.)

@billyjanitsch
Copy link
Contributor Author

enzymejs/react-shallow-renderer#93 will usually work but it will break backwards compatibility in certain situations. Specifically, it's possible to end up with a deps tree that npm considers valid but that will break at runtime, e.g.:

npm i https://github.com/NMinhNguyen/react-shallow-renderer#dccc711
npm i react@16

This produces the following tree, with conflicting versions of react-is:

├─┬ react@16.14.0
│ ├─┬ loose-envify@1.4.0
│ │ └── js-tokens@4.0.0
│ ├── object-assign@4.1.1
│ └─┬ prop-types@15.7.2
│   ├── loose-envify@1.4.0
│   ├── object-assign@4.1.1
│   └── react-is@16.13.1
└─┬ react-shallow-renderer@16.13.1 (git+https://github.com/NMinhNguyen/react-shallow-renderer.git#dccc7112e4606ea10a8125d8333ac5a86f5f3101)
  ├── object-assign@4.1.1
  └── react-is@17.0.0

In general, this will happen any time that a version of react-shallow-renderer that depends on ^16.12.0 || ^17.0.0 is installed before react-is@16 already exists in the tree. npm won't dedupe the dep later.

Maybe you consider this too unlikely to warrant fixing?

@gaearon
Copy link
Collaborator

gaearon commented Oct 20, 2020

Why do you think this would break at runtime? That module is stateless.

@gaearon
Copy link
Collaborator

gaearon commented Oct 20, 2020

Should be fixed.

@gaearon gaearon closed this as completed Oct 20, 2020
@billyjanitsch
Copy link
Contributor Author

Thanks; sorry for the assertive framing without context. I looked more closely at react-test-renderer and I see that it only ever uses react-is to test for specific element types so it won't break.

(The problem I was imagining is if it were using something like isValidElementType and the 17 element types eventually changed over time, e.g., by addition of new types, such that the shallow renderer would get assertions that would be inaccurate from the POV of React 16.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

2 participants