[miniflare] fix: make the magic proxy able to proxy objects containing functions

[dario-piotrowicz]

What this PR solves / how to test

This PR makes it so that "magic" proxy objects can contain functions, something that is not currently possible.

To test the changes in this PR you can try this minimal reproduction/example with both the latest version of miniflare and this PR's miniflare prerelease. You can see how the example errors with the former but succeeds with the latter.

Note

This improves the situation but the magic proxy's handling of functions is not yet perfect, for example functions inside arrays (e.g. return [() => 'Hello']) still don't work. Also functions returning functions do not work either.

In any case I believe that the changes here improve the situation and are unlikely to conflict with any fix for the above points, so I think that it should be safe to proceed with these changes (and address the other points separately).

Author has addressed the following

• Tests
  • TODO (before merge)
  • Included
  • Not necessary because:
• Changeset (Changeset guidelines)
  • TODO (before merge)
  • Included
  • Not necessary because:
• Public documentation
  • TODO (before merge)
  • Cloudflare docs PR(s): https://github.com/cloudflare/cloudflare-docs/pull/...
  • Not necessary because: bugfix

[changeset-bot]

🦋 Changeset detected

Latest commit: 1531565

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
miniflare	Patch
@cloudflare/pages-shared	Patch
@cloudflare/vitest-pool-workers	Patch
wrangler	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]

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/9008096780/npm-package-wrangler-5670

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9008096780/npm-package-wrangler-5670 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9008096780/npm-package-create-cloudflare-5670 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9008096780/npm-package-cloudflare-kv-asset-handler-5670

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9008096780/npm-package-miniflare-5670

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9008096780/npm-package-cloudflare-pages-shared-5670

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9008096780/npm-package-cloudflare-vitest-pool-workers-5670

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

---

wrangler@3.54.0 includes the following runtime dependencies:

Package	Constraint	Resolved
miniflare	workspace:*	3.20240419.0
workerd	1.20240419.0	1.20240419.0
workerd --version	1.20240419.0	2024-04-19

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

[petebacondarwin]

Is it worth adding tests that reads non-function properties off an object that also has functions?

[dario-piotrowicz]

Is it worth adding tests that reads non-function properties off an object that also has functions?

yeah that's a good shout, I'll look into adding that 🙂👍

[RamIdeas]

Can you make clear that this fix doesn't allow those functions to be callable

[petebacondarwin]

Is that true? JS RPC does allow methods and callback to be called I believe.

[RamIdeas]

This isn't JS RPC – it's the miniflare magic proxy

[petebacondarwin]

But isn't it using JS RPC under the hood? Perhaps I am misunderstanding.

[dario-piotrowicz]

@petebacondarwin no this is unrelated to JS RPC 🙂, it is using the magic proxy and saving such object on the heap (in workerd) so that they can be referenced later.

JS RPC is implemented in the same sort of way:

workers-sdk/packages/miniflare/src/workers/core/proxy.worker.ts

Lines 55 to 57 in 66bdad0

	if(value?.constructor?.name === 'RpcStub') {
	return false;
	}

but one is not necessary related to the other

[dario-piotrowicz]

@RamIdeas The functions are callable, as you can see from the unit tests below 👇

or am I misunderstanding your comment?

[dario-piotrowicz]

PS: I've updated the changeset as I can see that serialize here is not really the right word and can lead to confusion 🙂
(my changes are not actually serializing such objects but allowing us to handle them)

[RamIdeas]

Ah yeah sorry I didn't see any implementation changes so assumed you were simply allowing the object to be serialised whereas before it was prevented from proxying.

Am I right to now think that proxying objects with functions always worked? If so, do we know why we explicitly prevented it?

[dario-piotrowicz]

Am I right to now think that proxying objects with functions always worked?

No, they never worked, only the rpc stubs ones but that's because we're specifically handling them ad-hoc

If so, do we know why we explicitly prevented it?

The thing is that with my changes we check if an object contains functions and if so we mark as a non-plain object.
Basically my changes make the !isPlainObject(object) trigger here:

workers-sdk/packages/miniflare/src/workers/core/proxy.worker.ts

Lines 88 to 93 in c0b057b

	if ((type === "Object" && !isPlainObject(value)) || type === "Promise") {
	const address = this.nextHeapAddress++;
	this.heap.set(address, value);
	assert(value !== null);
	return [address, value?.constructor.name];
	}

making it so that the object doesn't get sent to devalue which would otherwise complain when trying to serialize it (saying that it doesn't know how to serialize functions).

[dario-piotrowicz]

@petebacondarwin I've added a non-function property usage to one of the tests (I was considering adding a test just for this but it felt a bit overkill... 🤔)

Please have a look and let me know if you're ok with this 🙏 🙂

[RamIdeas]

This PR needs rebasing ("Required Github CI checks have changed for workers-sdk. Please rebase on main to get the required jobs to run")