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 installing Wrangler using pnpm #339

Closed
wants to merge 1 commit into from
Closed

Conversation

Leetfs
Copy link

@Leetfs Leetfs commented Dec 11, 2024

issue: #338

test url: https://github.com/Leetfs-dev/test/actions/runs/12271068666/job/34238417617

dependencies:
  + pnpm 9.13.2 (9.15.0 is available)
Run Leetfs/wrangler-action@v1
🔍 Checking for existing Wrangler installation
📥 Installing Wrangler
🚀 Running Wrangler Commands
🏁 Wrangler Action completed

@Leetfs Leetfs requested review from a team as code owners December 11, 2024 07:28
@Leetfs
Copy link
Author

Leetfs commented Dec 28, 2024

@Maximo-Guk

Hi, could you please take a look at this PR when you have time? It's been pending for a while. Let me know if any changes are needed. Thanks.

@Maximo-Guk
Copy link
Member

Maximo-Guk commented Feb 19, 2025

Hi @Leetfs 👋, sorry for taking so long to respond!

I'm not sure if it's common for GitHub actions to install packages to the global scope, since it seems to me that now our action might be introducing some unintended side effects.

I think I would recommend users to instead use the official pnpm action https://github.com/pnpm/action-setup, and then invoke wrangler-action with your globally installed package.

What do you think?

@Leetfs
Copy link
Author

Leetfs commented Feb 19, 2025

Hi @Maximo-Guk , thanks for your reply

I retested the workflow, using pnpm/[email protected] and cloudflare/[email protected]

Please note:

  • pnpm does not allow adding dependencies directly to the workspace root directory by default
  • Need to use -g -w parameters to explicitly specify the installation location

I think it will be more convenient to install wrangler to the global path in the workflow, so use the -g parameter.

image

image

@Maximo-Guk
Copy link
Member

Maximo-Guk commented Feb 19, 2025

@Leetfs Are you able to install wrangler in the root package.json of your project? https://github.com/mtftm/test/blob/main/package.json#L14

"devDependencies": {
     ...
     "wrangler": "latest" // or pin the version, if you'd like
     ...
}

If you have wrangler specified in your root package.json, it should become available to the action as well, meaning you wouldn't even need to add wrangler to the global scope

@Leetfs
Copy link
Author

Leetfs commented Feb 19, 2025

@Maximo-Guk Sure ~ but I still recommend fixing this.

Perhaps I would suggest using the -w flag instead of modifying the package.json.

This might confuse developers who haven't specified wrangler in their package.json, as the workflow already includes a step to install wrangler.

Additionally, there are the following reasons:

  • The current behavior of pnpm is inconsistent with other package managers.
  • For example, the same project might work with npm but not with pnpm.
  • If wrangler is directly specified in package.json, then the automatic installation of wrangler by the wrangler-action becomes moot, which doesn't seem to be the intended behavior.

In summary, it is recommended to modify the installation command in response to the pnpm error.

Thank you for your attention.

@Maximo-Guk
Copy link
Member

Maximo-Guk commented Feb 19, 2025

I believe -w flag only works when you're inside of a pnpm workspace.

image

I definitely think we should document this better, and urge users to specify wrangler in their package.json if they're using a pnpm workspace

@Leetfs
Copy link
Author

Leetfs commented Feb 19, 2025

@Maximo-Guk Okay~Thank you for your attention to this PR.

@Leetfs Leetfs closed this Feb 19, 2025
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