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

chore: add react peer dependencies #1938

Merged
merged 6 commits into from Aug 31, 2022
Merged

chore: add react peer dependencies #1938

merged 6 commits into from Aug 31, 2022

Conversation

quisido
Copy link
Contributor

@quisido quisido commented Jul 16, 2022

All the packages in this repo require react and react-dom as peer dependencies, and react-spring does not provide them even though they are listed as dependencies. It needs to "pass the buck" to the consumer of react-spring in order to get rid of these warnings on install:

➤ YN0002: │ react-spring@npm:9.4.5 doesn't provide react (p45d7f), requested by @react-spring/core
➤ YN0002: │ react-spring@npm:9.4.5 doesn't provide react (p89414), requested by @react-spring/konva
➤ YN0002: │ react-spring@npm:9.4.5 doesn't provide react (pad289), requested by @react-spring/native
➤ YN0002: │ react-spring@npm:9.4.5 doesn't provide react (p67454), requested by @react-spring/three
➤ YN0002: │ react-spring@npm:9.4.5 doesn't provide react (p313ce), requested by @react-spring/web
➤ YN0002: │ react-spring@npm:9.4.5 doesn't provide react (p43935), requested by @react-spring/zdog
➤ YN0002: │ react-spring@npm:9.4.5 doesn't provide react-dom (pcccdb), requested by @react-spring/web
➤ YN0002: │ react-spring@npm:9.4.5 doesn't provide react-dom (p0095e), requested by @react-spring/zdog

This fix is validated by adding the following to .yarnrc:

packageExtensions:
  react-spring@*:
    peerDependencies:
      react: '*'
      react-dom: '*'

But I'd rather not do the YAML implementation because it's dead code if this fix is ever applied to the source, and other devs on the team won't understand maintaining this. I'd rather it be fixed at the source, hence the PR. :)

@vercel
Copy link

vercel bot commented Jul 16, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
react-spring-io ✅ Ready (Inspect) Visit Preview Aug 31, 2022 at 4:55PM (UTC)

Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

See comments

packages/react-spring/package.json Outdated Show resolved Hide resolved
@quisido
Copy link
Contributor Author

quisido commented Aug 3, 2022

I've updated it with the versions from @react-spring/core.

@quisido
Copy link
Contributor Author

quisido commented Aug 16, 2022

@joshuaellis Sorry to ping. It's been a few weeks. The changes were made. Is it good to 🐿?

@joshuaellis
Copy link
Member

No totally fair to ping me. I think you need to run yarn and push the new lockfile and then all three checks should pass 👍🏼 I can then double check tomorrow if they pass.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 19, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3c6585b:

Sandbox Source
spring-animating-auto Configuration
spring-card Configuration
spring-cards-stack Configuration
spring-chain Configuration
spring-css-keyframes Configuration
spring-draggable-list Configuration
spring-exit-before-enter Configuration
spring-flip-card Configuration
spring-goo-blobs Configuration
spring-image-fade Configuration
spring-list-reordering Configuration
spring-masonry Configuration
spring-multistage-transition Configuration
spring-notification-hub Configuration
spring-notification-hub Configuration
spring-notification-hub Configuration
initial-rocket Configuration
spring-simple-transition Configuration
spring-svg-filter Configuration
spring-slide Configuration
spring-tree Configuration
spring-viewpager Configuration

@quisido
Copy link
Contributor Author

quisido commented Aug 19, 2022

@joshuaellis Sorry for the delay. I've updated the lock file.

@joshuaellis joshuaellis changed the title add react peer dependencies chore: add react peer dependencies Aug 31, 2022
@joshuaellis joshuaellis merged commit ffc1c1b into pmndrs:master Aug 31, 2022
@eugenet8k
Copy link
Contributor

eugenet8k commented Sep 1, 2022

Thank you for the change. But it seems the values in peer dependencies are not correct. Now it is:

  "peerDependencies": {
     "react": "^16.8.0  || >=17.0.0 || >=18.0.0",
     "react-dom": "^16.8.0  || >=17.0.0 || >=18.0.0"
   }

but having >= twice does not make sense, if the version bigger than 18 it also will be bigger than 17. This online check tool demonstrates this

https://jubianchi.github.io/semver-check/#/^16.8.0%20%20||%20%3E%3D17.0.0%20||%20%3E%3D18.0.0/20

Did you perhaps want to have this syntax instead?

  "peerDependencies": {
     "react": "^16.8.0  || ^17.0.0 || ^18.0.0",
     "react-dom": "^16.8.0  || ^17.0.0 || ^18.0.0"
   }

this is more common for libraries as it allows to have any version within the major version range, but it won't apply to React v19. Your current version range would match any React version, even React v100.

CC @CharlesStover, @joshuaellis

@joshuaellis
Copy link
Member

This isn't released yet, feel free to make a PR and submit your findings so we can discuss separately.

eugenet8k added a commit to eugenet8k/react-spring that referenced this pull request Sep 1, 2022
* It targets React v16, v17 and v18, and only those

fixes pmndrs#1938
@eugenet8k
Copy link
Contributor

@joshuaellis I made a PR that failed checks:

➤ YN0028: │ -    react: ^16.11.0  || >=17.0.0 || >=18.0.0
➤ YN0028: │ +    react: ^16.8.0 || ^17.0.0 || ^18.0.0
➤ YN0000: │      three: ">=0.126"
➤ YN0000: │    languageName: unknown
➤ YN0000: │    linkType: soft
➤ YN0000: │  
➤ YN0000: │ 
➤ YN0028: │ The lockfile would have been modified by this install, which is explicitly forbidden.
➤ YN0000: └ Completed

It seems I am not allowed to modify yarn.lock file in my commit, but how to address this change in peerDependencies then?

@joshuaellis
Copy link
Member

@eugenet8k youd have to run yarn to modify the lock file which is fine because you're changing peer deps.

eugenet8k added a commit to eugenet8k/react-spring that referenced this pull request Sep 3, 2022
* It targets React v16, v17, and v18, and only those

fixes pmndrs#1938
@eugenet8k
Copy link
Contributor

@joshuaellis it seems I got successful checks this time, please review and thank you for all the work with this repo.

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.

None yet

3 participants