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

Support react 17 peer dep #32

Closed
wants to merge 1 commit into from
Closed

Support react 17 peer dep #32

wants to merge 1 commit into from

Conversation

bcomnes
Copy link

@bcomnes bcomnes commented Nov 5, 2020

Geting peer dep warnings. Everything still works though.

Geting peer dep warnings.  Everything still works though.
@mtmacdonald
Copy link

Also requesting for this to be merged please!

@Hypnosphi
Copy link
Contributor

@jamiebuilds can you please review

@sfcgeorge
Copy link

Since this is a polyfill it seems loosening the dependency to >= React 14 would be better. Even if React removes createContext again in a future version the polyfill would just start working again.

Currently with this overly strict dependency downstream projects like Reach Router (which uses create-react-context) and Storybook (which uses Reach) are failing to install using NPM v7 due to it now handling peerDependencies correctly.

@haysclark
Copy link

haysclark commented Nov 27, 2020

Since this is a polyfill it seems loosening the dependency to >= React 14 would be better. Even if React removes createContext again in a future version the polyfill would just start working again.

I believe that there is some wisdom behind being explicit, especially when it comes to JS dependencies (in this case "peerDependencies"); but I do agree it's rather subjective. 😉 My take-away from the React 17 release notes is that Facebook is preparing the community for API breaking changes coming in future versions of React. Knowing that future versions of React are not guaranteed to be backwards compatible, makes me lean away from the allure of >= React 14.

Just my two cents. ¯\(ツ)

@haysclark
Copy link

Since this is a polyfill it seems loosening the dependency to >= React 14 would be better. Even if React removes createContext again in a future version the polyfill would just start working again.

My take-away from the React 17 release notes is that Facebook is preparing the community for API breaking changes coming in future versions of React. Knowing that future versions of React are not guaranteed to be backwards compatible, makes me lean away from the allure of >= React 14.

@sfcgeorge Actually, I think you are correct that >= React 14 would be better for this particular repo. The fact that this library is a polyfill completely changes my feeling towards its future compatibility and maintenance. It just took me a little time to catch up with your insight. :) TLDR; Re-reading your comment above, as well as your feedback in reach/router helped me understand your perspective. 👏

@bcomnes
Copy link
Author

bcomnes commented Nov 28, 2020

All good points, just going with what will make things work, and the existing pattern, though changing it to > 14 would be better.

@haysclark
Copy link

@jamiebuilds Merge? Feedback? 😄

@sfcgeorge
Copy link

Thanks @haysclark, there's good arguments both ways. @bcomnes is right, keeping most things the same is the easiest way to get something merged, and "done is better than perfect" ☺️

@haysclark
Copy link

still waiting for @jamiebuilds

@revmischa
Copy link

Please, I beg of you, merge this!
It breaks storybook with react 17 if you use npm v7, which catches these broken peer deps

Could not resolve dependency:
peer react@"^0.14.0 || ^15.0.0 || ^16.0.0" from create-react-context@0.3.0
node_modules/@reach/router/node_modules/create-react-context
  create-react-context@"0.3.0" from @reach/router@1.3.4
  node_modules/@reach/router
    @reach/router@"^1.3.3" from @storybook/api@6.1.11

@bcomnes
Copy link
Author

bcomnes commented Jan 11, 2021

I reached out to Jamie on Twitter a while back but did not receive a response. Would anyone be willing to launch and maintain a maintenance fork of this dependency and attempt to land it downstream to reach/storybook ?

@Hypnosphi
Copy link
Contributor

I think I can

@Hypnosphi
Copy link
Contributor

Forked to https://github.com/Hypnosphi/create-react-context & published as @hypnosphi/create-react-context

I used >= 0.14.0 per suggestion by @sfcgeorge

@bcomnes
Copy link
Author

bcomnes commented Jan 11, 2021

I am no longer interested in maintaining this pull request. If anyone wants to offer up another one here, feel free. I will be switching to @Hypnosphi's fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants