docs: remove warning about packageManager and pnpm default #245
Conversation
| @@ -53,10 +52,6 @@ jobs: | |||
|
|
|||
| ## Configuration | |||
|
|
|||
| ### packageManager | |||
|
|
|||
| ⚠️ you must specify package manager. If not specified GH action assumes you are using npm and other managers might be failing. For example pnpm will fail with: `Cannot read properties of null (reading 'matches')` | |||
There was a problem hiding this comment.
I agree that we should drop this paragraph, but we should completely undocument the packageManager option. Instead of dropping this paragraph, could you please replace it with an a short blurb about packageManager option and how to use it (following the style for other options below). thanks
There was a problem hiding this comment.
The original intent here was mainly to just revert #242, but I've added a short blurb about packageManager under the "advanced usage" section since I figure most people won't ever need to touch this.
| @@ -31,7 +31,6 @@ jobs: | |||
| - name: Deploy | |||
| uses: cloudflare/wrangler-action@v3 | |||
| with: | |||
| packageManager: pnpm # you can omit this if you use npm | |||
There was a problem hiding this comment.
+1, we should shouldn't be forcing this on devs
There was a problem hiding this comment.
typo, and I assume you mean "shouldn't"? If so, agreed!
| @@ -53,10 +52,6 @@ jobs: | |||
|
|
|||
| ## Configuration | |||
|
|
|||
| ### packageManager | |||
|
|
|||
| ⚠️ you must specify package manager. If not specified GH action assumes you are using npm and other managers might be failing. For example pnpm will fail with: `Cannot read properties of null (reading 'matches')` | |||
remote ws
|
Thanks for the feedback @IgorMinar. I believe everything should be addressed now. Sadly the tests here will fail due to the necessary secrets not existing in the PR. Reference: #216 (comment) This PR only updates docs though, so should be safe to merge. |
|
Thanks James. This reads much better! I've merged the PR. |
This effectively reverts #242.
This is inferred by #190 and then fixed in #193, and I'm not really sure the "default" copy/pasted config should use
pnpm- that's definitely not the standard.You do not need to specify
packageManageras the docs now suggests, as long as you're running an up to date version.If there is a bug in the package manager detection, I'd suggest that should be reported and addressed separately.