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

[wrangler] fix: avoid getPlatformProxy logging twice that it is using vars defined in .dev.vars files #7947

Merged

Conversation

dario-piotrowicz
Copy link
Member

@dario-piotrowicz dario-piotrowicz commented Jan 28, 2025

Fixes #7944

when getPlatformProxy is called and it retrieves values from .dev.vars files, it logs twice a message like: Using vars defined in .dev.vars, the changes here make sure that in such cases this log only appears once

(Prerequisite for fixing opennextjs/opennextjs-cloudflare#288)


  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because: this functionality is already tested, the only noticeable change here is the removal of the extra terminal log, I am not sure if that can be reliably tested, I can look into that if we believe it beneficial but it feels not completely worth it to me
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: bug fix

@dario-piotrowicz dario-piotrowicz requested a review from a team as a code owner January 28, 2025 19:05
Copy link

changeset-bot bot commented Jan 28, 2025

🦋 Changeset detected

Latest commit: aec0e24

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

This PR includes changesets to release 2 packages
Name Type
wrangler Patch
@cloudflare/vitest-pool-workers 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

Copy link
Contributor

github-actions bot commented Jan 28, 2025

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/13019696632/npm-package-wrangler-7947

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13019696632/npm-package-wrangler-7947 dev path/to/script.js
Additional artifacts:

cloudflare-workers-bindings-extension:

wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13019696632/npm-package-cloudflare-workers-bindings-extension-7947 -O ./cloudflare-workers-bindings-extension.0.0.0-vdc1547143.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-vdc1547143.vsix

create-cloudflare:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13019696632/npm-package-create-cloudflare-7947 --no-auto-update

@cloudflare/kv-asset-handler:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13019696632/npm-package-cloudflare-kv-asset-handler-7947

miniflare:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13019696632/npm-package-miniflare-7947

@cloudflare/pages-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13019696632/npm-package-cloudflare-pages-shared-7947

@cloudflare/unenv-preset:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13019696632/npm-package-cloudflare-unenv-preset-7947

@cloudflare/vite-plugin:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13019696632/npm-package-cloudflare-vite-plugin-7947

@cloudflare/vitest-pool-workers:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13019696632/npm-package-cloudflare-vitest-pool-workers-7947

@cloudflare/workers-editor-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13019696632/npm-package-cloudflare-workers-editor-shared-7947

@cloudflare/workers-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13019696632/npm-package-cloudflare-workers-shared-7947

@cloudflare/workflows-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13019696632/npm-package-cloudflare-workflows-shared-7947

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


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20250124.0
workerd 1.20250124.0 1.20250124.0
workerd --version 1.20250124.0 2025-01-24

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

…ng vars defined in `.dev.vars` files

when `getPlatformProxy` is called and it retrieves values from `.dev.vars` files, it logs twice
a message like: `Using vars defined in .dev.vars`, the changes here make sure that in such cases
this log only appears once
@dario-piotrowicz dario-piotrowicz force-pushed the dario/7944/getPlatformProxy-dev-vars-double-log branch from 5f9463c to aec0e24 Compare January 28, 2025 21:15
@dario-piotrowicz dario-piotrowicz added the e2e Run wrangler e2e tests on a PR label Jan 28, 2025
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point being that getMiniflareOptionsFromConfig() now also reads the .dev.vars so there is no need to do it again in getPlatformProxy()?

@petebacondarwin petebacondarwin merged commit f57bc4e into main Jan 29, 2025
33 of 34 checks passed
@petebacondarwin petebacondarwin deleted the dario/7944/getPlatformProxy-dev-vars-double-log branch January 29, 2025 11:55
@dario-piotrowicz
Copy link
Member Author

The point being that getMiniflareOptionsFromConfig() now also reads the .dev.vars so there is no need to do it again in getPlatformProxy()?

yes, exactly 🙂👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run wrangler e2e tests on a PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

🐛 BUG: getPlatformProxy logs Using vars defined in .dev.vars twice
2 participants