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.

Sign up for GitHub

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

Add React / DOM 17 to peer deps #432

Closed
wants to merge 1 commit into from
+22 −16
Closed

Add React / DOM 17 to peer deps #432

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions package.json
View file
Original file line number Diff line number Diff line change
@@ -27,8 +27,8 @@
"testURL": "http://localhost"
},
"peerDependencies": {
"react": "15.x || 16.x || 16.4.0-alpha.0911da3",
"react-dom": "15.x || 16.x || 16.4.0-alpha.0911da3"
"react": "15.x || 16.x || 16.4.0-alpha.0911da3 || ^17.0.1",
"react-dom": "15.x || 16.x || 16.4.0-alpha.0911da3 || ^17.0.1"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexandernst. Yuck... unfortunately, I think you are just adding to this mess. A Git blame for this line shows that the previous author wrote a commit message: "Yeah I dunno". My 2 cents is that the peer dependencies should be cleaned up, using just one wild-card format. ^x.x.x is more commonly used and also explicit. This would be far cleaner:

"peerDependencies": {
  "react": "^15.0.0 || ^16.0.0 || ^17.0.0",
  "react-dom": "^15.0.0 || ^16.0.0 || ^17.0.0"
},

For reference, here are some similar repo's addressing this issue: material-ui.

Alternatively, react-router doesn't even need to change any code to support React 17; however, I agree with Martin Fowler and others that "To Be Explicit" is better.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way to do it is "15 - 17"

},
"devDependencies": {
"babel-cli": "^6.26.0",
@@ -54,8 +54,8 @@
"prettier": "^1.13.4",
"pretty-bytes": "^4.0.2",
"pretty-quick": "^1.6.0",
"react": "16.12.0",
"react-dom": "16.12.0",
"react": "16.12.0 || ^17.0.1",
"react-dom": "16.12.0 || ^17.0.1",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since they don't include 15 or 16 for devdependencies, is there any reason to match against 16?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who are "they" and where don't they include 15 or 16 to devdeps?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"They" are the people who own this package.json and I mispoke, they didn't do 15 || 16, so why now do 16 || 17 for devDependencies?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a fail safe. It doesn't hurt to leave it there since 16 and 17 are compatible (which is not the case for 15 and 16)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compatible in what way? They're breaking versions, so at least some incompatibility must exist.

Either way I guess it's not a blocker.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SCROLL DOWN.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2020-11-26 at 9 08 00 AM

Copy link

@Hypnosphi Hypnosphi Nov 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, there's no point in having 16 || 17 in devDependencies since yarn will always install v17, see yarn.lock change

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can see that Rollup is set up to removed react and react-dom from the bundle, thus the devDependancy really does not need to change.

Suggested change
"react-dom": "16.12.0 || ^17.0.1",
"react-dom": "16.12.0 || ^17.0.1",

Copy link

@haysclark haysclark Nov 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hole point of React 17

@alexandernst I think you mean, "The whole point...". 😉

"react-test-renderer": "16.12",
"render-markdown-js": "^1.3.0",
"rollup": "^0.56.3",
@@ -80,7 +80,7 @@
"printWidth": 80
},
"dependencies": {
"create-react-context": "0.3.0",
"create-react-context": "^0.3.0",
"invariant": "^2.2.3",
"prop-types": "^15.6.1",
"react-lifecycles-compat": "^3.0.4"
28 changes: 17 additions & 11 deletions yarn.lock
View file
Original file line number Diff line number Diff line change
@@ -4166,15 +4166,14 @@ rc@^1.2.7:
minimist "^1.2.0"
strip-json-comments "~2.0.1"

[email protected]:
version "16.12.0"
resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-16.12.0.tgz#0da4b714b8d13c2038c9396b54a92baea633fe11"
integrity sha512-LMxFfAGrcS3kETtQaCkTKjMiifahaMySFDn71fZUNpPHZQEzmk/GiAeIT8JSOrHB23fnuCOMruL2a8NYlw+8Gw==
"[email protected] || ^17.0.1":
version "17.0.1"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this

resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-17.0.1.tgz#1de2560474ec9f0e334285662ede52dbc5426fc6"
integrity sha512-6eV150oJZ9U2t9svnsspTMrWNyHc6chX0KzDeAOXftRa8bNeOKTTfCJ7KorIwenkHd2xqVTBTCZd79yk/lx/Ug==
dependencies:
loose-envify "^1.1.0"
object-assign "^4.1.1"
prop-types "^15.6.2"
scheduler "^0.18.0"
scheduler "^0.20.1"

react-is@^16.8.1, react-is@^16.8.6:
version "16.12.0"
@@ -4196,14 +4195,13 @@ [email protected]:
react-is "^16.8.6"
scheduler "^0.18.0"

[email protected]:
version "16.12.0"
resolved "https://registry.yarnpkg.com/react/-/react-16.12.0.tgz#0c0a9c6a142429e3614834d5a778e18aa78a0b83"
integrity sha512-fglqy3k5E+81pA8s+7K0/T3DBCF0ZDOher1elBFzF7O6arXJgzyu/FW+COxFvAWXJoJN9KIZbT2LXlukwphYTA==
"[email protected] || ^17.0.1":
version "17.0.1"
resolved "https://registry.yarnpkg.com/react/-/react-17.0.1.tgz#6e0600416bd57574e3f86d92edba3d9008726127"
integrity sha512-lG9c9UuMHdcAexXtigOZLX8exLWkW0Ku29qPRU8uhF2R9BN96dLCt0psvzPLlHc5OWkgymP3qwTRgbnw5BKx3w==
dependencies:
loose-envify "^1.1.0"
object-assign "^4.1.1"
prop-types "^15.6.2"

read-pkg-up@^1.0.1:
version "1.0.1"
@@ -4622,6 +4620,14 @@ scheduler@^0.18.0:
loose-envify "^1.1.0"
object-assign "^4.1.1"

scheduler@^0.20.1:
version "0.20.1"
resolved "https://registry.yarnpkg.com/scheduler/-/scheduler-0.20.1.tgz#da0b907e24026b01181ecbc75efdc7f27b5a000c"
integrity sha512-LKTe+2xNJBNxu/QhHvDR14wUXHRQbVY5ZOYpOGWRzhydZUqrLb2JBvLPY7cAqFmqrWuDED0Mjk7013SZiOz6Bw==
dependencies:
loose-envify "^1.1.0"
object-assign "^4.1.1"

select@^1.1.2:
version "1.1.2"
resolved "https://registry.yarnpkg.com/select/-/select-1.1.2.tgz#0e7350acdec80b1108528786ec1d4418d11b396d"