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

[Runner VM Bug] behavior of {}.constructor === Object does not match node behavior #141

Closed
monoclex opened this issue Jan 3, 2022 · 2 comments
Labels
behaviour mismatch Different behaviour to Workers runtime fixed-in-next Fixed in next release

Comments

@monoclex
Copy link

monoclex commented Jan 3, 2022

Minimal Repro

import { VMScriptRunner } from "@miniflare/runner-vm";

console.log("node     :", {}.constructor === Object);

const runner = new VMScriptRunner();
const results = await runner.run(globalThis, {
  code: `console.log("miniflare:", {}.constructor === Object);`,
});

Miniflare's current, incorrect output

node     : true
miniflare: false

Ideal, expected output

node     : true
miniflare: true

Rationale for fixing bug

I came across this bug while attempting to use the React Material UI library, @mui/material. After copying their code and debugging cryptic errors, it led me to this crucial difference between a typical node environment and miniflare that prevented the code from running correctly.

This specific behavior is used in deepmerge of Material UI, which is use to merge default theme values together. Specifially, this check can be found here: https://github.com/mui-org/material-ui/blob/a9903917f919092f80d84075f39fb51d51f241f2/packages/mui-utils/src/deepmerge.ts#L1-L3. The bug from a user standpoint (when using Material UI) looks as such:

TypeError: Cannot read properties of undefined (reading 'fontWeightBold')
    at styles (/tmp/functionsWorker.js:23779:41)
    at styles (/tmp/functionsWorker.js:23720:29)
    at GlobalStyles.globalStyles (/tmp/functionsWorker.js:18528:26)
    at handleInterpolation (/tmp/functionsWorker.js:15228:22)
    at serializeStyles2 (/tmp/functionsWorker.js:15471:20)
    at /tmp/functionsWorker.js:15772:24
    at Object.render (/tmp/functionsWorker.js:15561:16)
    at ReactDOMServerRenderer2.render (/tmp/functionsWorker.js:10150:50)
    at ReactDOMServerRenderer2.read2 [as read] (/tmp/functionsWorker.js:9990:37)
    at renderToString2 (/tmp/functionsWorker.js:10571:35)

Software versions

> ./node_modules/.bin/wrangler2 --version
0.0.0-485e0dc
> node --version
v17.3.0

"@miniflare/runner-vm": {
  "version": "2.0.0-rc.3",
@monoclex monoclex changed the title [Runner VM Bug] Plain objects do not have Object constructor [Runner VM Bug] behavior of {}.constructor === Object does not match node behavior Jan 3, 2022
@mrbbot
Copy link
Contributor

mrbbot commented Jan 3, 2022

Hey! 👋 Quite a few other people have faced this issue:

As explained here #109 (comment), this behaviour was originally added to make Rust development easier. However, I'm beginning to think making this the default was a bad idea... 😅

While we can still making breaking changes before the full v2 release, I'll probably switch this over to requiring something like a --proxy-primitive flag, and leave Object untouched by default. This still has the problem that prototype/constructor/instanceof checks for Objects created outside the sandbox (e.g. KV/cached responses) would fail, but I think that's less likely to be an issue, since the return types are always known in those cases.

mrbbot added a commit that referenced this issue Jan 3, 2022
This was initially added to make it easier to run Rust workers in
Miniflare, as `wasm-bindgen` frequently generates this code. However,
the implementation caused `Object` prototype/constructor checks to
fail in JavaScript. The previous behaviour can be restored with the
`--proxy-primitive`/`miniflare.proxy_primitive_instanceof`/
`proxyPrimitiveInstanceOf` CLI/`wrangler.toml`/API option.

Closes #109
Closes #137
Closes #141
Closes cloudflare/workers-sdk#91
@mrbbot mrbbot added behaviour mismatch Different behaviour to Workers runtime fixed-in-next Fixed in next release labels Jan 3, 2022
@mrbbot
Copy link
Contributor

mrbbot commented Jan 3, 2022

Hey! 👋 Miniflare 2.0.0-rc.5 has just been released, which implements this fix. You can find the full changelog here. Should be in wrangler soon.

@mrbbot mrbbot closed this as completed Jan 3, 2022
mrbbot added a commit that referenced this issue Jan 7, 2022
This was initially added to make it easier to run Rust workers in
Miniflare, as `wasm-bindgen` frequently generates this code. However,
the implementation caused `Object` prototype/constructor checks to
fail in JavaScript. The previous behaviour can be restored with the
`--proxy-primitive`/`miniflare.proxy_primitive_instanceof`/
`proxyPrimitiveInstanceOf` CLI/`wrangler.toml`/API option.

Closes #109
Closes #137
Closes #141
Closes cloudflare/workers-sdk#91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behaviour mismatch Different behaviour to Workers runtime fixed-in-next Fixed in next release
Projects
None yet
Development

No branches or pull requests

2 participants