-
Notifications
You must be signed in to change notification settings - Fork 772
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
fix: allow getPlatformProxy
to handle RPC calls returning objects
#6301
Conversation
🦋 Changeset detectedLatest commit: 5a4fb43 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
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/10161265027/npm-package-wrangler-6301 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6301/npm-package-wrangler-6301 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10161265027/npm-package-wrangler-6301 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10161265027/npm-package-create-cloudflare-6301 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10161265027/npm-package-cloudflare-kv-asset-handler-6301 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10161265027/npm-package-miniflare-6301 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10161265027/npm-package-cloudflare-pages-shared-6301 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10161265027/npm-package-cloudflare-vitest-pool-workers-6301 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
7dbe330
to
7a4e327
Compare
I've been baffled by the same issue for a couple weeks now and the prerelease above fixes it for me 👍 |
0181e43
to
d258ac1
Compare
after talking to Dario async, seems like this PR introduces a small regression (see #6235 (comment)). Let's hold off merging it until I discuss this with our team |
2054447
to
c65d040
Compare
allow the `getPlatformProxy` to handle RPC calls returning objects by making sure that the miniflare magic proxy can take into account methods set via symbols (in the case or RPC objects specifically `Symbol.dispose` and `Symbol.asyncDispose`) additionally add more RPC test coverage in miniflare
c65d040
to
d044599
Compare
@CarmenPopoviciu I've refactored the PR and now it's not introducing any regression at all 😄 If should be pretty good to go! 😃 Please give it another quick review if you would 🙏 |
What this PR solves / how to test
Fixes #6285
Fixes the first point of #6235
Author has addressed the following
As part of #5922 I've removed the code that we had (implemented by @penalosa) that dropped the problematic
dispose
andasyncDispose
properties fromRpcProperty
s (see here), as I thought that such operations were no longer necessary (and there were no tests showing that removing them was breaking something) but it turns out that they are 😓, so I am reinstating the old code back and adding tests to make sure we don't re-regress on this 🙂