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: detect pnpm-workspace.yml and append -w when installing wrangler #207

Closed
wants to merge 1 commit into from

Conversation

sam-goodwin
Copy link

The wrangler-action does not currently support pnpm workspaces.

See: #181 (comment)

It fails with:

📥 Installing Wrangler
  /home/runner/setup-pnpm/node_modules/.bin/pnpm add wrangler@* -w
   ERR_PNPM_ADDING_TO_ROOT  Running this command will add the dependency to the workspace root, which might not be what you want - if you really meant it, make it explicit by running this command again with the -w flag (or --workspace-root). If you don't want to see this warning anymore, you may set the ignore-workspace-root-check setting to true.
Error: The process '/home/runner/setup-pnpm/node_modules/.bin/pnpm' failed with exit code 1
Error: 🚨 Action failed

This change detects a pnpm workspace by also looking for pnpm-workspace.yml file and then selects an install script that appends -w

@@ -19,6 +19,10 @@ const PACKAGE_MANAGERS = {
install: "pnpm add",
exec: "pnpm exec",
},
"pnpm-workspace": {
install: "pnpm add -w",
exec: "pnpm exec",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think pnpm exec needs to be adjusted.

@sam-goodwin
Copy link
Author

I noticed the tests only check that the right config is selected. It would be better to actually run the action within the test folders instead.

Copy link
Contributor

@1000hz 1000hz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! This solution adds a new user-facing value of packageManager: pnpm-workspace distinct from the existing pnpm value. While in most cases it won't be necessary for the user to explicitly specify a value since it will be inferred from the file system, we should still avoid introducing a new value to our public interface to work around this issue.

Let's rework this to dynamically include the -w argument for pnpm when necessary. One way to do this would be to make values of the PACKAGE_MANAGERS object optionally be a function, which we'd invoke before returning in getPackageManager().

@lrapoport-cf
Copy link
Contributor

hey @sam-goodwin :) we haven't heard from you in a while. if you'd still like to get this PR landed, please update in response to @1000hz 's feedback. otherwise, we'll close this PR until you're ready to revisit :) thanks!

@lrapoport-cf
Copy link
Contributor

hey @sam-goodwin :) we haven't heard from you so we'll close this PR for now, but please feel free to re-open when you're ready to revisit. thanks!

@jacob-shuman
Copy link

jacob-shuman commented May 13, 2024

This is currently preventing me from using the wrangler action in my pnpm monorepo. I think this can be automatically inferred (at least for pnpm) by checking for a pnpm-workspace.yaml/pnpm-workspace.yml file in the cwd.

Edit: sorry missed that from the original comment 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants