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

add local deps to packages #8

Closed
wants to merge 2 commits into from

Conversation

avidreder
Copy link
Contributor

@avidreder avidreder commented Nov 11, 2023

Ran into issues on WSL running ./gradlew npmInstall. The issue was that if a package A had a local dependency on package B, if package B had a local dependency on package C, the install command would fail due to A trying to install C.

@zach-herridge can you confirm that the install/build process on your machine works as expected with these changes?

@avidreder avidreder marked this pull request as ready for review November 11, 2023 14:16
@zach-herridge
Copy link
Member

I'm not really following this explanation of the error. I might be missing something but the gradle command is only running npm install nothing more. Just because A has a dep on B and B on C doesn't mean that running npm install in A will try to install C in A.

The gradle commands do run the npm install commands in the right order, but adding deps to the packages doesn't effect this order so if that was the problem this PR wouldn't fix it.

The changes are fine since those deps will probably eventually be needed in those places anyways but I would be less willing to add unused deps in the future so this fix will probably not work in all cases.

@avidreder
Copy link
Contributor Author

avidreder commented Nov 11, 2023

The actual example is that insights has a dependency on sage-common, which has the dependency on ts-ratchet. There are similar dependency graphs that also result in this error for me (example-character-plugin > echo-common > sage-common)

The order is correct, as install for ts-ratchet and sage-common happen prior to insights. The error that is surfaced is:
image

I don't really have a good explanation as to why npm believes it needs to install ts-ratchet. I would expect that if npm saw the peer dependency of sage-common as not satisfied, it would present a different error, rather than try to install.

@avidreder
Copy link
Contributor Author

I tried switching the install command to use yarn, and it succeeded, with a few interesting warnings:

> Task :src:echo-app:npmInstall
yarn install v1.22.19
warning package.json: No license field
warning echo-app@1.0.0: No license field
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
warning " > echo-common@1.0.3" has incorrect peer dependency "ggg-api@file:../../../../.cache/yarn/v6/npm-echo-common-1.0.3-a3a012fd-e577-4bac-870e-87b59e520f99-1699722881078/node_modules/ggg-api".
warning " > echo-common@1.0.3" has incorrect peer dependency "react@file:../../../../.cache/yarn/v6/npm-echo-common-1.0.3-a3a012fd-e577-4bac-870e-87b59e520f99-1699722881078/node_modules/echo-app/node_modules/react".
warning " > echo-common@1.0.3" has incorrect peer dependency "react-dom@file:../../../../.cache/yarn/v6/npm-echo-common-1.0.3-a3a012fd-e577-4bac-870e-87b59e520f99-1699722881078/node_modules/echo-app/node_modules/react-dom".
warning " > echo-common@1.0.3" has incorrect peer dependency "sage-common@file:../../../../.cache/yarn/v6/npm-echo-common-1.0.3-a3a012fd-e577-4bac-870e-87b59e520f99-1699722881078/node_modules/sage-common".
warning " > echo-common@1.0.3" has incorrect peer dependency "ts-ratchet@file:../../../../.cache/yarn/v6/npm-echo-common-1.0.3-a3a012fd-e577-4bac-870e-87b59e520f99-1699722881078/node_modules/ts-ratchet".
warning " > ggg-api@1.0.3" has incorrect peer dependency "ggg-api@file:../../../../.cache/yarn/v6/npm-ggg-api-1.0.3-01363e8d-fa47-4ed7-8dbd-60e1118998df-1699722881080/node_modules/ggg-api".
warning " > ggg-api@1.0.3" has incorrect peer dependency "react@file:../../../../.cache/yarn/v6/npm-ggg-api-1.0.3-01363e8d-fa47-4ed7-8dbd-60e1118998df-1699722881080/node_modules/echo-app/node_modules/react".
warning " > ggg-api@1.0.3" has incorrect peer dependency "react-dom@file:../../../../.cache/yarn/v6/npm-ggg-api-1.0.3-01363e8d-fa47-4ed7-8dbd-60e1118998df-1699722881080/node_modules/echo-app/node_modules/react-dom".
warning " > ggg-api@1.0.3" has incorrect peer dependency "sage-common@file:../../../../.cache/yarn/v6/npm-ggg-api-1.0.3-01363e8d-fa47-4ed7-8dbd-60e1118998df-1699722881080/node_modules/sage-common".
warning " > ggg-api@1.0.3" has incorrect peer dependency "ts-ratchet@file:../../../../.cache/yarn/v6/npm-ggg-api-1.0.3-01363e8d-fa47-4ed7-8dbd-60e1118998df-1699722881080/node_modules/ts-ratchet".
warning " > sage-common@1.0.3" has incorrect peer dependency "ts-ratchet@file:../../../../.cache/yarn/v6/npm-sage-common-1.0.3-ea250237-befb-4aa5-993f-c026414c4254-1699722881099/node_modules/ts-ratchet".
warning " > postcss-loader@7.3.3" has unmet peer dependency "webpack@^5.0.0".
[4/4] Building fresh packages...
Done in 6.93s.
> Task :src:insights:npmInstall
yarn install v1.22.19
info No lockfile found.
[1/4] Resolving packages...
warning aws-sdk > querystring@0.2.0: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
warning aws-sdk > url > querystring@0.2.0: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
[2/4] Fetching packages...
[3/4] Linking dependencies...
warning " > sage-common@1.0.3" has unmet peer dependency "@react-rxjs/core@^0.10.7".
warning " > sage-common@1.0.3" has unmet peer dependency "@react-rxjs/utils@^0.9.7".
warning " > sage-common@1.0.3" has unmet peer dependency "object-hash@^3.0.0".
warning " > sage-common@1.0.3" has unmet peer dependency "ts-ratchet@file:../../../../.cache/yarn/v6/npm-sage-common-1.0.3-47694994-bbbe-47c7-a41d-f065beb8517e-1699722895345/node_modules/ts-ratchet".
warning Workspaces can only be enabled in private projects.
[4/4] Building fresh packages...
success Saved lockfile.
Done in 9.01s.```

@zach-herridge
Copy link
Member

That's interesting it works with Yarn, those warnings are all fine those packages shouldn't be meeting any of those deps. Wonder why Yarn is able to setup the link while npm fails. Cool find

@avidreder
Copy link
Contributor Author

avidreder commented Nov 11, 2023

There are two other working solutions I found:

  • adding the --legacy-peer-deps flag to the install command for insights and other packages with issues. This will prevent npm from trying to install the peer dependencies of the project
  • trying to use yarn instead of npm

Let me know what you think @zach-herridge

@zach-herridge
Copy link
Member

I think --legacy-peer-deps is probably the best option in the short term, exploring yarn/other ideas would take more time and probably isn't worth it in the middle of @C3ntraX's work on HMR.

@avidreder
Copy link
Contributor Author

@zach-herridge I wasn't able to reproduce this on my Macbook, so given that it's not reproducible on the Windows build either, I'm going to close this. I will verify some things about my WLS2 setup, and if other developers run into issues further down the road I can pick it back up.

@avidreder avidreder closed this Nov 12, 2023
avidreder pushed a commit to avidreder/poestack-sage that referenced this pull request Dec 3, 2023
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.

2 participants