Skip to content

Always log when delegating to local wrangler install#2391

Merged
mrbbot merged 1 commit into
mainfrom
bcoll/local-delegation-logging
Dec 14, 2022
Merged

Always log when delegating to local wrangler install#2391
mrbbot merged 1 commit into
mainfrom
bcoll/local-delegation-logging

Conversation

@mrbbot

@mrbbot mrbbot commented Dec 12, 2022

Copy link
Copy Markdown
Contributor

Closes #2302.

When a global wrangler command is executed in a package directory with wrangler installed locally, the command is redirected to the local wrangler install. Whilst sometimes useful, this behaviour is contrary to most other npm CLI packages (and npx wrangler ... should already call the local version if detected). It also makes debugging issues more confusing, as it's not clear which wrangler version is being called. This PR replaces a debug log we already had for this with a regular log, so users (and wrangler developers 😅) know what's going on.

@mrbbot
mrbbot requested a review from a team as a code owner December 12, 2022 11:05
@changeset-bot

changeset-bot Bot commented Dec 12, 2022

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 4860c2d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions

github-actions Bot commented Dec 12, 2022

Copy link
Copy Markdown
Contributor

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/runs/3675287393/npm-package-wrangler-2391

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/prs/2391/npm-package-wrangler-2391

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/runs/3675287393/npm-package-wrangler-2391 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/runs/3675287393/npm-package-cloudflare-pages-shared-2391

@mrbbot
mrbbot force-pushed the bcoll/local-delegation-logging branch from 3213ed5 to 04ae257 Compare December 12, 2022 11:08
@petebacondarwin

Copy link
Copy Markdown
Contributor

LOL - eab9542

@mrbbot

mrbbot commented Dec 12, 2022

Copy link
Copy Markdown
Contributor Author

🤣 Ah... I would say this feature is more confusing than the debug message, as it's not what other Node CLI tools do, but we should probably discuss this. 😅

@codecov

codecov Bot commented Dec 12, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2391 (4860c2d) into main (21bab6b) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2391      +/-   ##
==========================================
+ Coverage   70.71%   70.74%   +0.03%     
==========================================
  Files         153      153              
  Lines        9751     9751              
  Branches     2550     2550              
==========================================
+ Hits         6895     6898       +3     
+ Misses       2856     2853       -3     
Impacted Files Coverage Δ
packages/wrangler/src/git-client.ts 81.25% <0.00%> (+4.16%) ⬆️
...ackages/wrangler/src/__tests__/helpers/mock-bin.ts 100.00% <0.00%> (+5.26%) ⬆️

@mrbbot
mrbbot force-pushed the bcoll/local-delegation-logging branch from 04ae257 to 4860c2d Compare December 12, 2022 11:23
@rozenmd

rozenmd commented Dec 12, 2022

Copy link
Copy Markdown
Contributor

Part of me wants to revisit this feature, but we implemented so teams could be sure that their commands run on the version specified in their package.json

@petebacondarwin

Copy link
Copy Markdown
Contributor

The problem is that some people just like to use the global version of stuff.

This is really not a good idea for NodeJS packages. We should be strongly encouraging people to install the version locally and then use npx or equivalent package manager tools to run it.

As long as people use global commands then the best we can do is this sneaky trick.

@JacobMGEvans

Copy link
Copy Markdown
Contributor

The problem is that some people just like to use the global version of stuff.

This is really not a good idea for NodeJS packages. We should be strongly encouraging people to install the version locally and then use npx or equivalent package manager tools to run it.

As long as people use global commands then the best we can do is this sneaky trick.

+1

@mrbbot
mrbbot merged commit 19525a4 into main Dec 14, 2022
@mrbbot
mrbbot deleted the bcoll/local-delegation-logging branch December 14, 2022 20:11
@github-actions github-actions Bot mentioned this pull request Dec 14, 2022
@penalosa

Copy link
Copy Markdown
Contributor

Just to note for future reference, I approved this so we'd get the debug log which will make it clear what Wrangler is doing (and encourage people to use npx for direct invocation)—it seems most of the discussion is about the "sneaky trick" which this debug log points out.

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.

🚀 Feature Request: log when using local Wrangler version

5 participants