Add React / DOM 17 to peer deps

[alexandernst]

No description provided.

[bcomnes]

Getting peer dependency warnings without this.

[Hypnosphi]

@blainekasten can you please review?

[krainboltgreene]

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

[alexandernst]

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

[krainboltgreene]

"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?

[alexandernst]

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)

[krainboltgreene]

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

Either way I guess it's not a blocker.

[krainboltgreene]

SCROLL DOWN.

[krainboltgreene]

[Hypnosphi]

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

[haysclark]

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",

[haysclark]

hole point of React 17

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

[Hypnosphi]

this

[haysclark]

@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.

[noahtallen]

Another way to do it is "15 - 17"

[sfcgeorge]

For some extra context, people who are having issues are likely using NPM v7 where their project will completely fail to install due to peerDependency conflicts. I think in v6 or Yarn they basically ignore peerDepencencies (giving a warning at best), so anyone using those won't have failed installations (even though technically the dependency tree is invalid). Most JS projects are facing teething pains adapting to NPM v7 now that it finally does the right thing so no blame there's just some tweaks and getting used to it. Good luck to all! 🚀

Personally I wonder if simply >= 15 would be more pragmatic. React doesn't do breaking changes without deprecation often so it seems preferable to assume future versions will be compatible rather than arbitrarily blocking numerous downstream projects from being able to upgrade to the latest React. I'd rather it actually be broken than just refusing to install for no good reason. In an ideal world projects would be really on top of their dependencies and test them all using build matrixes (the first React 17 RC came out 4 months ago) but open source is free and people have limited time so only the biggest projects could be expected to do that. Nonetheless, that's for the maintainers to decide; explicit or loose is fine as long as there's some sort of update for React 17 😄

I think there's another blocker though because Reach Router strictly depends on create-react-context which has it's own strict dependency on React 16 and under. AKA I don't think this PR will actually fix Reach Router's React 17 support until create-react-context is similarly updated to support React 17, and then Reach Router needs to have a looser / updated dependency on create-react-context.

Finally for anyone Googling, Storybook v6.1 supposedly supports React 17 but actually doesn't due to Reach Router / create-react-context's peer dependencies.

[haysclark]

Most JS projects are facing teething pains adapting to NPM v7 now that it finally does the right thing so no blame there's just some tweaks and getting used to it. Good luck to all! 🚀

@sfcgeorge I just wanted to clarify that I was referring to git blame, but maybe I'm reading into your word choice too much. ;) I completely agree that blame is a very "loaded" word, which is unfortunate and I apologize if I caused any offense. My intent was to share my process of using git blame to do a little code archeology to better understand the intent of previous authors for those lines of code.

[alexandernst]

Not interested in following this anymore. Please feel free to copy this MR and submit it on your behalf.

[haysclark]

@alexandernst Why bother opening a PR for an Open Source project if you are not willing to take the feedback of the community?

[alexandernst]

I just lost interest in using this particular library (dropped the dependency from my project). As I said, feel free to take my MR, make any changes that you're willing to apply and submit it.
Me closing this MR has nothing to do with any feedback, it's just that I'm not interested anymore in this library and I'm not willing to spend anymore time on it.

[shilman]

Finally for anyone Googling, Storybook v6.1 supposedly supports React 17 but actually doesn't due to Reach Router / create-react-context's peer dependencies.

@sfcgeorge Storybook 6.1 supports React 17, just not with NPM 7's stricter enforcement of peer dependencies. If you install with yarn or npm7 using --legacy-peer-deps it works just fine.