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.

Sign up for GitHub

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

Jump to bottom

Add Warning about browser rendering when running locally #7533

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add Warning about browser rendering when running locally #7533

wants to merge 1 commit into from

Conversation

danielgek
Copy link

@danielgek danielgek commented Dec 12, 2024 •
edited by CarmenPopoviciu
Loading

Fixes #BRAPI-120.

Add warning when running browser rendering locally

  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because: this just adds a warning
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because: this just adds a warning
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: this just adds a warning

@danielgek danielgek requested a review from a team as a code owner December 12, 2024 18:15
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 12, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 257a2b3

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

@danielgek danielgek force-pushed the dleal/BRAPI-120-add-br-local-warning branch 2 times, most recently from 9f810da to c09902c Compare December 12, 2024 18:19
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 12, 2024 •
edited
Loading

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/12371058411/npm-package-wrangler-7533

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371058411/npm-package-wrangler-7533 dev path/to/script.js
Additional artifacts: 
wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371058411/npm-package-cloudflare-workers-bindings-extension-7533 -O ./cloudflare-workers-bindings-extension.0.0.0-v03c8f6dda.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v03c8f6dda.vsix
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371058411/npm-package-create-cloudflare-7533 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371058411/npm-package-cloudflare-kv-asset-handler-7533
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371058411/npm-package-miniflare-7533
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371058411/npm-package-cloudflare-pages-shared-7533
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371058411/npm-package-cloudflare-vitest-pool-workers-7533
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371058411/npm-package-cloudflare-workers-editor-shared-7533
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371058411/npm-package-cloudflare-workers-shared-7533
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371058411/npm-package-cloudflare-workflows-shared-7533

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.20241205.0
workerd 1.20241205.0 1.20241205.0
workerd --version 1.20241205.0 2024-12-05

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

CarmenPopoviciu
CarmenPopoviciu reviewed Dec 17, 2024
View reviewed changes
packages/wrangler/src/dev/miniflare.ts
@@ -951,6 951,10 @@ export async function buildMiniflareOptions(
}
}

if (config.bindings.browser) {
logger.warn("Browser render is not supported locally use `--remote`");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.warn("Browser render is not supported locally use `--remote`");
logger.warn("Browser render is not supported locally. Please use `wrangler dev --remote` instead.");

Copy link
Contributor

@CarmenPopoviciu CarmenPopoviciu Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why is this handled in miniflare.ts? This type of warnings and stdout logging is generally managed at command handler level. Which command(s) is/are targeted by this warning?

@CarmenPopoviciu CarmenPopoviciu force-pushed the dleal/BRAPI-120-add-br-local-warning branch from c09902c to 257a2b3 Compare December 17, 2024 10:25
@CarmenPopoviciu
Copy link
Contributor

Tests not necessary because: this just adds a warning

We usually test for this kind of stdout. See here for example. So we would def like to cover this warning as well by a unit test 🙏

@CarmenPopoviciu CarmenPopoviciu added the awaiting reporter response Needs clarification or followup from OP label Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarmenPopoviciu CarmenPopoviciu CarmenPopoviciu left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
awaiting reporter response Needs clarification or followup from OP
Projects
workers-sdk
Status: Untriaged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danielgek @CarmenPopoviciu