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

Allow future React versions in peer dependencies #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Hypnosphi
Copy link
Contributor

Replaces #32

@mdodge-ecgrow
Copy link

mdodge-ecgrow commented Jan 12, 2021

Why is there no issues tab in this repo?

@krainboltgreene
Copy link

@Hypnosphi I think this is probably a mistake to not pin to major versions. You don't want to give the false sense that it's safe when they release a new change when it's not.

@Hypnosphi
Copy link
Contributor Author

Hypnosphi commented Jan 24, 2021

See motivation here: #32 (comment) reach/router#432 (comment)

@mikestopcontinues
Copy link

I think the loose deps style makes more sense for the reasons @Hypnosphi points out, but I'd happily take a more strict definition if it meant this gets released.

@dietergeerts
Copy link

Versions should definitely be restrictive in the sense that when a new version of a peer dependency comes out, it first need to be tested with it to make sure it work with it, then the versioning can be updated to include this newer version.

I came across this as I encounter this in the Storybook packages that uses this, but they themselves set react 17 as peer dependency. So that makes it like very hard and unreliable, as technically, they are not telling the truth.

Can this be updated so tests are also done with the 17 version and then merged and released?

@mikestopcontinues

This comment has been minimized.

@Hypnosphi
Copy link
Contributor Author

Hypnosphi commented Apr 30, 2021

@dietergeerts this is a polyfill library. It doesn't really do anything on React 16.3+ (e.g. where React.createContext is available)

https://github.com/jamiebuilds/create-react-context/blob/master/src/index.js#L5

@dietergeerts
Copy link

@Hypnosphi , I don't use this directly, it's a deep dependency of "@storybook/react", and as I use pnpm and check on correction of peer dependencies, I know have errors on version used. That's why I landed here. So you are basically saying that the package that uses this in the dependency chain actually doesn't need it anymore when using react 17, and thus could upgrade it's dependencies to mark it as optional, or to just remove it when ditching previous version?

@Hypnosphi
Copy link
Contributor Author

upgrade it's dependencies to mark it as optional

Unfortunately not, because it uses import createContext from "create-react-context", and the check for the presence of React.createContext is made inside create-react-context package. See also reach/router#436 (comment)

@Brian-McBride
Copy link

Is this repo just not maintained anymore? It really seems odd that React 17 isn't properly supported.
The Gatsby guys are going off the rails with this

moduleNameMapper: {
    "^@reach/router(.*)": "<rootDir>/node_modules/@gatsbyjs/reach-router$1",
  },

I am assuming Gatsby flattens the Reach Router into the static links so they only need to worry about it in Jest testing?

Is there a recommended router lib that everyone is migrating to?

@tikakwork
Copy link

hi, is it possible to merge this commit to master in order to use this library in react 17 ?

@davidtung383
Copy link

hi, is it possible to merge this commit to master in order to use this library in react 17 ?

I want to know too, hope to support react 17 for this library

@CWSites
Copy link

CWSites commented Oct 27, 2021

Some movement here would be great.

warning "@react-theming/storybook-addon > @storybook/addon-devkit > @storybook/addons > @storybook/api > @reach/router > [email protected]" has incorrect peer dependency "react@^0.14.0 || ^15.0.0 || ^16.0.0".

@Methuselah96
Copy link

Methuselah96 commented Oct 27, 2021

Should be fixed when Storybook v6.4.0 is released thanks to storybookjs/storybook#16440.

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

Successfully merging this pull request may close these issues.