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

Add Warning about browser rendering when running locally #7533

Merged

Conversation

danielgek
Copy link
Contributor

@danielgek danielgek commented Dec 12, 2024

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
Copy link

changeset-bot bot commented Dec 12, 2024

🦋 Changeset detected

Latest commit: 90d78a3

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
Copy link
Contributor

github-actions bot commented Dec 12, 2024

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/12601808065/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/12601808065/npm-package-wrangler-7533 dev path/to/script.js
Additional artifacts:
wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12601808065/npm-package-cloudflare-workers-bindings-extension-7533 -O ./cloudflare-workers-bindings-extension.0.0.0-v4cad5d8ac.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v4cad5d8ac.vsix
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12601808065/npm-package-create-cloudflare-7533 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12601808065/npm-package-cloudflare-kv-asset-handler-7533
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12601808065/npm-package-miniflare-7533
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12601808065/npm-package-cloudflare-pages-shared-7533
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12601808065/npm-package-cloudflare-unenv-preset-7533
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12601808065/npm-package-cloudflare-vitest-pool-workers-7533
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12601808065/npm-package-cloudflare-workers-editor-shared-7533
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12601808065/npm-package-cloudflare-workers-shared-7533
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12601808065/npm-package-cloudflare-workflows-shared-7533

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


wrangler@3.99.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20241218.0
workerd 1.20241230.0 1.20241230.0
workerd --version 1.20241230.0 2024-12-30

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

@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
@danielgek danielgek force-pushed the dleal/BRAPI-120-add-br-local-warning branch from 0225a40 to ed5e621 Compare January 3, 2025 11:11
@danielgek danielgek force-pushed the dleal/BRAPI-120-add-br-local-warning branch from ed5e621 to 147d855 Compare January 3, 2025 11:28
@emily-shen emily-shen removed the awaiting reporter response Needs clarification or followup from OP label Jan 3, 2025
@emily-shen
Copy link
Contributor

Agh so I had a look at why tests were failing. Turns out resolved.bindings is a record with binding name as the key, so the error wasn't actually throwing.
Anyway this should be the right check and passes locally for me
if(extractBindingsOfType("browser", resolved.bindings).length && !resolved.dev.remote)

@danielgek
Copy link
Contributor Author

Agh so I had a look at why tests were failing. Turns out resolved.bindings is a record with binding name as the key, so the error wasn't actually throwing. Anyway this should be the right check and passes locally for me if(extractBindingsOfType("browser", resolved.bindings).length && !resolved.dev.remote)

Nice, also i left some suggestions on working, would be nice if they can go in as well

And Thanks for all the help!

@emily-shen emily-shen merged commit 755a27c into cloudflare:main Jan 3, 2025
26 checks passed
@workers-devprod workers-devprod mentioned this pull request Jan 3, 2025
penalosa pushed a commit that referenced this pull request Jan 10, 2025
* Warn about browser rendering when running locally

* fix warning check

* update snapshot

---------

Co-authored-by: emily-shen <69125074+emily-shen@users.noreply.github.com>
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.

3 participants