Skip to content

add "increase_websocket_message_size" compat flag to InspectorProxyWorker #6424

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
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.

[email protected] 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 + vite-plugin e2e tests on a PR label Aug 6, 2024
penalosa
penalosa reviewed Aug 6, 2024
View reviewed changes
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyjessop andyjessop andyjessop approved these changes

@penalosa penalosa penalosa left review comments

Assignees
No one assigned
Labels
e2e Run wrangler + vite-plugin 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