-
-
Notifications
You must be signed in to change notification settings - Fork 941
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: should not output warning info with non-top-level directory structure #7382
base: main
Are you sure you want to change the base?
Conversation
💖 Thanks for opening this pull request! 💖 |
80c932c
to
553f210
Compare
553f210
to
4f9f936
Compare
@zkochan can you help review this PR? |
I don't get it, the warning message is correct, those fields will be ignored, so why should it not be printed? |
@zkochan Yeah, this error message is fine under normal circumstances. But in a monorepo file structure like below
the
The purpose of this is to prevent the existence of node_modules in the root directory from causing phantom dependencies just like what Rush.js do. Since we did write these fields in a workspace not in root, there is no problem with this printing information for pnpm, but in fact since we create a virtual root package.json file and copy all So I would like to ask if you can add a npmrc configuration like What suggestions do you have for this situation? @zkochan |
ok, but in the temp directory there is still a package.json in the same directory where the lockfile is. So, where do you get the warning. I don't think you would get it in the temp directory. |
In the temp directory's installation process, the common folder is a normal workspace as same as other workspaces. We just automatically copy the pnpm fields to the temp package.json. The warning is still printed since the common folder with pnpm fields in package.json does existed just like a normal workspace. @zkochan |
Still don't understand where the issue is coming from. This repo is the same as your temp workspace: https://github.com/zkochan/pnpm-issue-7382 Run install in it, no warning is printed. |
https://github.com/Timeless0911/pnpm-issue-7382 see this repo, you can run |
The warning is still valid from pnpm's perspective. Rush should either pass pnpm some option to disable the warning, or use a field that pnpm doesn't read, then copy the settings to the root package.json. |
From pnpm's perspective I totally agree your opinion. In order to ensure the uniformity of using related pnpm fields, using a field that pnpm doesn't read seems not a good idea. I prefer the former solution to pass some option to disable the warning. Can you add a config named |
Some directory structure like below does not have workspace-root to be at the top level.
Rushjs and some other monorepo tools prefer this non-top-level structures to reduce the phantom dependence problem in root node_modules.
And in this mode, we usually configure
pnpm.overrides/resolutions
in a workspace that withpnpm-workspace.yaml
in. Then we make a temp directory and create a virtual root withpnpm.overrides/resolutions
being copied to it.In this directory structure, when we execute
pnpm install
, logs likeWARN The field "pnpm" was found in apps/web/package.json. This will not take effect. You should configure "pnpm" at the root of the workspace instead.
will be outputed.In this PR, I want to make this warning log not display in this directory structure. I just use
lodash.equal
to compare two object to determine this situation.@await-ovo can you help deal with this situation? Sincere thanks to you~