search

cloudflare / wrangler-action

🧙‍♀️ easily deploy cloudflare workers applications using wrangler and github actions
Apache License 2.0
1.18k stars 152 forks source link

(Attempt 2) - Use existing wrangler installation when appropriate #269

Closed AdiRishi closed 3 months ago

AdiRishi commented 3 months ago

:warning: This PR is still under development and not ready for review, it has been made early to discuss potential implementation strategies

Overview

This PR attempts to bring back #235 and fix the fatal flaw that caused it to be reverted.

Summary of what went wrong with the last PR

The code which checks for an existing installation runs npx wrangler --version to determine if wrangler is installed (or the equivilent command for other package managers). What I missed was that this command will automatically install the latest version of wrangler if no version is installed 🙃

This means that by accident wrangler-action now always will install the latest wrangler if you don't specify wranglerVersion in the config. This is a breaking change and bug, since we meant for it to keep it's old behavior and install version 3.13.2 by default.

Strategies to fix the bug

This is where things get tricky. Our task is as follows

Goal: Find out two key pieces of information

  1. Is there an existing wrangler installation?
  2. What exact version of wrangler is installed?

We need to do the above while supporting 6 different package managers which all have different sets of CLI commands

  1. npm
  2. yarn v1
  3. yarn v3
  4. yarn v4
  5. pnpm
  6. bun

I've looked through several options, and all of them have their pros and cons. I'm going to list down the various strategies from the most simple implementation to the most difficult. I would love to have a dicsussion on which one we should pick before I go and implement something.

Naively check package.json for version

This involves simply loading the package.json file in code and checking the dependencies and devDependencies objects for wrangler.

Pros: Simply to implement Cons:

  1. We have no way of knowing what exact version is installed if package.json has something like ^ or ~.
  2. There is also a slight risk that even though wrangler is present in package.json it wasn't installed fully or correctly.

Execute wrangler directly from node_modules/.bin

If wrangler is sucessfully installed, in all package managers* we will find it's executable linked to node_modules/.bin. We can then run it directly from here and do the same checks as before.

Pros: Simple to implement Cons:

  1. This will not work if the user is using Yarn PnP as their installation strategy. It requires nodeLinker: node-modules to be enabled
  2. This is somewhat of a hack, and could break if package managers decide to change their behavior

Use various package manager commands to determine installation

Package managers have various commands that list installed packages. npm and pnpm and bun have ls, yarn has why. These commands could be executed and we can capture and parse the output to figure out what wrangler version is installed.

Pros: 100% assurance that installation was successful Cons:

  1. The downside is that there is very little consistency between outputs from all package managers. Yarn makes things worse as we have to consider yarn 1, 3 and 4, with PnP varients.
  2. We would have to use a combination of different commands per package manager. Very complex implementation.
Maximo-Guk commented 3 months ago

Hey @AdiRishi 👋! I think it may be as simple as simply disabling the auto-installation behaviour of npx to at least have this work with the npm package manager.

npx has the --no-install flag, I tried it out on a reproduction repository here and it seemed to work as expected.

Only tricky thing is I'm not sure if all the package managers have similar options, I assume that they do, but need to look into it further!

Maximo-Guk commented 3 months ago

Added a PR https://github.com/cloudflare/wrangler-action/pull/271 which does the above! Let me know what you think!

AdiRishi commented 3 months ago

@Maximo-Guk I was definitely going down a much more complex route. I saw that --no-install was not present in each package manager and figured I'd need a different approach.

However. you did the smart thing, which was use slightly different execution strategies for each package manager. I think your way is much better than what I was suggesting haha 😅

The only option from the one I wrote out I would possibly consider is executing the binary directly from node_modules/.bin. But again, I think your PR does it in a much better way.

I'll close this one out in favor of yours.