-
Notifications
You must be signed in to change notification settings - Fork 12
Support pnpm package manager resolution #7
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
base: master
Are you sure you want to change the base?
Conversation
Add detection and command configuration for pnpm in mkPackageManagerCmds() (src/util.ts)
| install: "pnpm install", | ||
| run: "pnpm run", | ||
| add: "pnpm add", | ||
| cmd: "pnpm dlx", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| cmd: "pnpm dlx", | |
| cmd: "pnpm", |
To make it similar to yarn, I think it would be better for this to be just pnpm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! However, pnpm dlx should not be replaced by pnpm:
-
pnpm fooattempts to run a local script or a binaryfoofromnode_modules/.bin, and will fail if the package isn't already installed. -
pnpm dlx foodownloads and runs packagefoowithout installing it locally, similar tonpx. This is the expected behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the current usage, I think the correct thing to do would be to use the local binary for this.
It is used to build (the user should have their platform installed locally) and to sync (the user should have the CLI locally and we should use the version they have). From what I understand, npx checks if the package is installed locally before executing it (in this case it should)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand, npx checks if the package is installed locally before executing it (in this case it should)
Using pnpm dlx also ensures that package is installed locally before executing it.
In code, "cmd" property is used for plasmic sync and next/gatsby binaries. It's absolutely correct usage for "pnpm dlx" here:
await exec(
// pnpm dlx plasmic ...
`${pm.cmd} plasmic sync --projects '${this.args.projectId}:${this.args.projectApiToken}' --yes`,
this.opts
);
// ...
case "nextjs":
// pnpm dlx next ...
await exec(`${pm.cmd} next build`, this.opts);
await exec(`${pm.cmd} next export`, this.opts);
dir = "out";
break;
case "gatsby":
// pnpm dlx gatsby ...
await exec(`${pm.cmd} gatsby build`, this.opts);
dir = "public";
break;"pnpm dlx" is exact pnpm mirror for "npx" command. In other words, if we want “npx-style” behavior (i.e. fetch if missing, then run), we should use "pnpm dlx" and neither "pnpm" nor any other pnpm command. Therefore, I recommend keeping "pnpm dlx", as using "pnpm" will just break the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is, if I have a specific next version in the project I want that version to be used. All the packages you listed will be in the users package.json and that specific version should be used.
Can you link me any docs that specify that pnpm dlx will prefer the local binary over downloading the latest version from the internet? From what I saw, it said that i would always download from the internet before executing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid issues, I think it should be fine to change the npm one to be npm exec
Add detection and command configuration for pnpm in mkPackageManagerCmds() (src/util.ts)