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

Feat(unstable_dev): Add option to enable update check #2982

Closed
wants to merge 1 commit into from
Closed

Feat(unstable_dev): Add option to enable update check #2982

wants to merge 1 commit into from

Conversation

Skye-31
Copy link
Contributor

@Skye-31 Skye-31 commented Apr 3, 2023

Remade in #4347

Old PR Summary:

Fixes #2956.

What this PR solves / how to test:

Wrangler makes a HTTP request to the NPM registry, checking for updates to inform end users. While this makes sense in regular commands, it (generally) has little place in it's API, particularly when this is primarily used for tests.

This pull request adds an experimental option which disables the update check, and optionally allows users to enable it. (There are cases for this, such as pages dev)

Associated docs issue(s)/PR(s):

  • N/A

Author has included the following, where applicable:

Reviewer has performed the following, where applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

@Skye-31 Skye-31 requested review from a team as code owners April 3, 2023 21:51
@changeset-bot
Copy link

changeset-bot bot commented Apr 3, 2023

🦋 Changeset detected

Latest commit: 9aab3c9

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

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

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

@GregBrimble
Copy link
Member

GregBrimble commented Apr 3, 2023

Any reason we wouldn't just always disable this check when using the API?

@Skye-31
Copy link
Contributor Author

Skye-31 commented Apr 3, 2023

Pages Dev uses the API, so we need a way to re-turn it on 😅
There's also the case for people using this in their own libraries/tools (let's say, a tool that lets you deploy code to various platforms, and interacts with their respective libraries), who may actually want it to be on

@GregBrimble
Copy link
Member

Feels like the update checker should just be part of the main CLI entrypoint, and then all underlying logic (e.g. APIs) don't have it, but I defer to the core Wrangler team for their thoughts.

@Skye-31
Copy link
Contributor Author

Skye-31 commented Apr 4, 2023

I thought that too before implementing, but I looked at the usages for printWranglerBanner and it happened in every command separately instead of the entrypoint (my guess is it's conditionally to do with --json flags supported by some commands?) so I just modified the existing implementation. Happy to refactor it, but it felt like it could have some wider unintended implications

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

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/workers-sdk/runs/4617428842/npm-package-wrangler-2982

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

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

Or you can use npx with this latest build directly:

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

Note that these links will no longer work once the GitHub Actions artifact expires.

@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #2982 (9aab3c9) into main (1fc414e) will increase coverage by 0.03%.
Report is 404 commits behind head on main.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2982      +/-   ##
==========================================
+ Coverage   73.55%   73.58%   +0.03%     
==========================================
  Files         167      167              
  Lines       10497    10506       +9     
  Branches     2805     2809       +4     
==========================================
+ Hits         7721     7731      +10     
+ Misses       2776     2775       -1     
Files Coverage Δ
packages/wrangler/src/api/dev.ts 58.88% <ø> (ø)
packages/wrangler/src/dev.tsx 85.00% <100.00%> (ø)
packages/wrangler/src/pages/dev.ts 18.77% <ø> (ø)
packages/wrangler/src/index.ts 83.83% <25.00%> (-0.64%) ⬇️

... and 3 files with indirect coverage changes

@Skye-31
Copy link
Contributor Author

Skye-31 commented Apr 5, 2023

Fixed the checks issue

@JacobMGEvans
Copy link
Contributor

This whole Programmatic API of Wrangler is in the works for a Systems Design & Spec, I appreciate the PR. This is likely the correct approach in the future for that major work (#2982 (comment))

Cc: @RamIdeas @admah

@Skye-31 Skye-31 deleted the skye/unstable-dev-no-update-fetch branch August 7, 2023 16:08
@Skye-31 Skye-31 restored the skye/unstable-dev-no-update-fetch branch October 2, 2023 15:12
@Skye-31 Skye-31 reopened this Oct 2, 2023
@Skye-31 Skye-31 closed this Oct 2, 2023
@Skye-31 Skye-31 deleted the skye/unstable-dev-no-update-fetch branch November 2, 2023 14:23
@jakubadamw
Copy link

This would be a really helpful flag. We use a lot of wrangler invocations in our CI workflows and thus waste quite a bit of time for the update checks.

@Skye-31
Copy link
Contributor Author

Skye-31 commented Nov 6, 2023

Remade in #4347 - this branch was very out of date

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

Successfully merging this pull request may close these issues.

🎉 Feature Request: Don't run an update check in unstable_dev
4 participants