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

Remove prop-types from peer dependencies in our JS libraries #145

Open
2 tasks
adamstankiewicz opened this issue Nov 10, 2022 · 1 comment
Open
2 tasks
Labels
bug Report of or fix for something that isn't working as intended

Comments

@adamstankiewicz
Copy link
Member

adamstankiewicz commented Nov 10, 2022

In some of our JS repos, we treat prop-types as a peer dependency, when it really shouldn't be (as confirmed by the docs for prop-types itself). Rather, it should be a regular dependency that ships as part of the library.

There's been a couple occasions where prop-types as a peer dependency causes unnecessary peer dependency conflicts that could be avoided had prop-types been considered a regular dependency.

The aforementioned docs on prop-types that provide guidance on how to depend on the package: https://github.com/facebook/prop-types#how-to-depend-on-this-package

AC

@abdullahwaheed abdullahwaheed assigned Mashal-m and unassigned Mashal-m Nov 15, 2022
@arbrandes arbrandes added the bug Report of or fix for something that isn't working as intended label Dec 9, 2022
@ishahroz
Copy link
Member

ishahroz commented Dec 23, 2022

Hi, I was working on this issue and is blocked by another error I came across while trying to move prop-types from peerDependencies to dependencies. Following are the major steps / observations related to it:

  1. I picked frontend-platform as my JS repo to move prop-types from peerDependencies to dependencies in the package.json file. Simply npm-uninstalled prop-types from peerDependencies (using --save-dev flag) and then installed it again in dependencies context (using --save flag), so that package-lock.json file remains in sync with the changes.
  2. After the above change, frontend-platform build was successful, all the test cases are passing and its front-end preview is also working fine. PR Link
  3. As part of the next step, I tried to test these changes across the frontend-platform's consumer repos. I picked frontend-app-publisher as my consumer repo, created frontend-platform build locally, copied this compressed build in the root directory of frontend-app-publisher and lastly referred and installed frontend-platform local package build by using this notation: "@edx/frontend-platform": "file:edx-frontend-platform-1.0.0-semantically-released.tgz".
  4. Next, when I am trying to test cases for frontend-app-publisher repo, all of them are failing with two error primarily (attached screenshot as the reference):
  • Cannot find module '@edx/frontend-platform/auth' from 'src/setupTest.js'
  • 'regenerator-runtime/runtime' import error

Screenshot 2022-12-23 at 12 42 59 PM

  1. For the first (cannot find module) error, I found out in the previous version of [email protected], the auth module was in the root directory, and later file structure was changed and all the modules were moved into src folder. Simply, changed from '@edx/frontend-platform/auth' to '@edx/frontend-platform/src/auth' in 'src/setupTest.js' and it was referencing it fine.
  2. For the second error, I have tried many applicable solutions related to webpack configuration, browerlist configuration and all of them seem to be not working. Link 1 Link 2.

Even trying the above two workarounds, I am still facing the same issue as what can be seen in the attached screenshot. What are your thoughts on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Report of or fix for something that isn't working as intended
Projects
Status: Blocked
Development

No branches or pull requests

4 participants