Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

add "increase_websocket_message_size" compat flag to InspectorProxyWorker #6424

Merged
merged 9 commits into from
Aug 13, 2024
Merged

add "increase_websocket_message_size" compat flag to InspectorProxyWorker #6424

merged 9 commits into from
Aug 13, 2024

Conversation

RamIdeas
Copy link
Contributor

@RamIdeas RamIdeas commented Aug 6, 2024 •
edited
Loading

What this PR solves / how to test

Fixes #5297

TODO:

Regression test can be seen failing here https://github.com/cloudflare/workers-sdk/actions/runs/10324844174/job/28585175390?pr=6424#step:6:322

You can see the test failing and then passing in this screencast:
https://github.com/user-attachments/assets/e718a39c-b506-43d9-9c57-e23aa7cfcab3

Author has addressed the following

  • Tests
    • TODO (before merge)
    • Included
    • Not necessary because:
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required / Maybe required
    • Not required because:
  • Changeset (Changeset guidelines)
    • TODO (before merge)
    • Included
    • Not necessary because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Not necessary because:

@RamIdeas RamIdeas requested a review from a team as a code owner August 6, 2024 10:22
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Aug 6, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 413ec16

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 6, 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/10368770809/npm-package-wrangler-6424

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10368770809/npm-package-wrangler-6424 dev path/to/script.js
Additional artifacts: 
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10368770809/npm-package-create-cloudflare-6424 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10368770809/npm-package-cloudflare-kv-asset-handler-6424
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10368770809/npm-package-miniflare-6424
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10368770809/npm-package-cloudflare-pages-shared-6424
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10368770809/npm-package-cloudflare-vitest-pool-workers-6424
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10368770809/npm-package-cloudflare-workers-editor-shared-6424
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10368770809/npm-package-cloudflare-workers-shared-6424

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

wrangler@3.70.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240806.0
workerd 1.20240806.0 1.20240806.0
workerd --version 1.20240806.0 2024-08-06

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

@RamIdeas RamIdeas marked this pull request as draft August 6, 2024 10:31
@workers-devprod workers-devprod added the e2e Run wrangler e2e tests on a PR label Aug 6, 2024
@RamIdeas RamIdeas force-pushed the ws-message-size branch 3 times, most recently from 6674357 to 1d94722 Compare August 12, 2024 15:50
@RamIdeas RamIdeas marked this pull request as ready for review August 12, 2024 15:52
andyjessop
andyjessop approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@andyjessop andyjessop left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. A few minor comments, but otherwise nice work 👍

packages/wrangler/e2e/startWorker.test.ts
vi.spyOn(console, "info").mockImplementation(mockConsoleLogImpl);
vi.spyOn(console, "log").mockImplementation(mockConsoleLogImpl);

const LARGE_STRING = "This is a large string" + "z".repeat(2 ** 20);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I would probably be more explicit here: 1_000_001, or something like that. I had to log out 2^20 to see what it came to, so this is more cognitive overhead than just writing out the number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though the limit is explained as "1MB" it is actually 2 ** 20. I was getting unreliable results with 1_000_000 (probably because the rest of the json message sometimes pushed it over the threshold) – it wasn't enough to ensure the message size was over the limit 100% of the time. I will add a comment in a follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh got it, np 👍

packages/wrangler/e2e/startWorker.test.ts

await vi.waitFor(
async () => {
await expect(consoleApiMessages).toMatchObject([
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the await necessary here? I think this call is synchronous, so you may be able to do-away with the inner async/await.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Will fix in a follow-up PR.

@RamIdeas RamIdeas merged commit 3402ab9 into main Aug 13, 2024
20 checks passed
@RamIdeas RamIdeas deleted the ws-message-size branch August 13, 2024 11:56
@workers-devprod workers-devprod mentioned this pull request Aug 13, 2024
Merged
@penalosa penalosa mentioned this pull request Aug 22, 2024
Closed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@andyjessop andyjessop andyjessop approved these changes

@penalosa penalosa penalosa left review comments

Assignees
No one assigned
Labels
e2e Run wrangler e2e tests on a PR
Projects
workers-sdk
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🐛 BUG: VSCode debugger doesn't work on Wrangler 3.19 and up
4 participants
@RamIdeas @andyjessop @penalosa @workers-devprod