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

Fix peer deeps for monorepos #673

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

salvoravida
Copy link

By submitting a PR to this repository, you agree to the terms within the Auth0 Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.

Description

This PR addresses the issue of peer dependencies involving @types/react for monorepos. Essentially, if you're using this package in a monorepo that includes applications utilizing both React 17 and React 18, it fails to accurately fetch the appropriate types.

References

Include any links supporting this change such as a:

  • GitHub Issue/PR number addressed or fixed
  • Auth0 Community post
  • StackOverflow post
  • Support forum thread
  • Related pull requests/issues from other repos

If there are no references, simply delete this section.

Testing

Describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

Please include any manual steps for testing end-to-end or functionality not covered by unit/integration tests.

Also include details of the environment this PR was developed in (language/platform/browser version).

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not the default branch

@frederikprijck
Copy link
Member

Thanks for reaching out and providing this PR. Can you help me understand the need to add this as a peer dependency? What if people aren't using typescript? Why would they need to pull in the same peer deps?

@salvoravida
Copy link
Author

salvoravida commented Nov 27, 2023

Thanks for reaching out and providing this PR. Can you help me understand the need to add this as a peer dependency? What if people aren't using typescript? Why would they need to pull in the same peer deps?

@frederikprijck I added peerDependenciesMeta with optional flag.

@frederikprijck
Copy link
Member

Thanks for updating the metadata, but I am more curious to understand why you think we should need to have a peer dependency on types, as well as why it solves the issue in your project and why you can not solve it outside of our SDK (in your own project) instead.

Would you be able to share a minimal reproduction of the issue for us to look into and see what's causing it and better understand the issue and solution?

@salvoravida
Copy link
Author

salvoravida commented Nov 27, 2023

Thanks for updating the metadata, but I am more curious to understand why you think we should need to have a peer dependency on types, as well as why it solves the issue in your project and why you can not solve it outside of our SDK (in your own project) instead.

Would you be able to share a minimal reproduction of the issue for us to look into and see what's causing it and better understand the issue and solution?

Apologies, as setting up a test monorepo containing two React applications (one with React 17 and another with React 18) will require considerable time. I suggest examining the approaches used by other libraries to understand the concept better. It's important to note that a library supporting multiple versions of React also needs to have corresponding peer dependencies in the types/*. This allows the consumer application to select the appropriate types.

https://github.com/radix-ui/primitives/blob/main/packages/react/arrow/package.json

@frederikprijck
Copy link
Member

frederikprijck commented Dec 2, 2023

Please understand this also takes the same effort for us to reproduce and look into.

Happy to look at something as soon as possible once something that reproduces it is provided. If not, this will need to wait due to other priorities at the moment and will need to get back to this at a later point (but we will still need more information about your setup to ensure we can reproduce this efficiently).

We don't generally just merge PRs with little context (the issue template isnt filled), and we like to understand what code we are adding by seeing it fix a certain issue and ensure we are onboard with the fix, I hope you understand that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more info needed This issue is waiting for more information in order to progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants