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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

instanceof not working on object returned by Rewire #8446

Closed
georgezlei opened this issue May 9, 2019 · 9 comments
Closed

instanceof not working on object returned by Rewire #8446

georgezlei opened this issue May 9, 2019 · 9 comments

Comments

@georgezlei
Copy link

馃悰 Bug Report

instanceof doesn't work as expected in Jest when being used on an object returned from Rewire. Please check out codes below.

I thought it was the problem of Rewire. However this issue can't be reproduced under pure node.js with Rewire.

I checked the prototype of the remote object. It just looks like a normal object. Don't understand what made it unrecognizable by instanceof.

To Reproduce

file rewired.test.js

const rewire = require('rewire')
const code = rewire('./rewired')

const here = {}
const there = code.__get__('another')

test('test', () => {
  expect(here instanceof Object).toBeTruthy()
  expect(there instanceof Object).toBeTruthy()
})

file rewired.js

const another = {}

Run npx envinfo --preset jest

Paste the results here:

  System:
    OS: macOS 10.14.4
    CPU: (8) x64 Intel(R) Core(TM) i7-4960HQ CPU @ 2.60GHz
  Binaries:
    Node: 12.1.0 - /usr/local/bin/node
    Yarn: 1.7.0 - /usr/local/bin/yarn
    npm: 6.9.0 - /usr/local/bin/npm
  npmPackages:
    jest: ^24.7.1 => 24.7.1 
@jeysal
Copy link
Contributor

jeysal commented May 9, 2019

Confirmed. Probably vm related, following prints false:

const vm =require('vm')
vm.runInNewContext(`
const rewire = require("rewire");
const code = rewire("./rewired");

const here = {};
const there = code.__get__("another");

console.log(there.__proto__ === here.__proto__);
`, {require,console})

@georgezlei
Copy link
Author

Thank you for the analysis. An issue (#27632) has been submitted to node.js.

@georgezlei
Copy link
Author

Engineers from node.js think it is a feature that all built-in objects such as Object are newly created in each context. So it is normal that the references to it are different.

Using vm.runInNewContext is like creating a new browser window. All JavaScript has builtins like Object and Array but they are created for their individual context. That means that each browser window has it's own Object instance. These contexts are also called realm. It is possible to cross the boundary of such context so one context may execute JavaScript code created in the other context. But the objects created in one now have a different base Object builtin instance than the objects created in the other context. instanceof checks can therefore not work over the boundary.

That is exactly what happens here: here is created in the context created by vm while there is a reference to an object created in a different context. The instanceof check tests if those objects are of the same instance of the current context and that's the vm one. Thus the second instanceof check fails.

@georgezlei
Copy link
Author

Here are some test codes to simulate this issue.

const vm = require('vm')
const code = `
const rewire = require("rewire");
const code = rewire("./rewired")

const there = code.__get__("there");
const obj = code.__get__("Object");

console.log(there instanceof obj);
console.log(there instanceof Object);
`;

eval(code)
vm.runInNewContext(code, {require, console})
vm.runInNewContext(code, {require, console, Object})

Result is

true
true
true
false
true
true

It seems we can have two workarounds for this issue before it can be fixed. One is to use the base classes from rewire instead of the builtin base classes. The other one is Jest to create an option is pass some object to new vm context.

@jeysal
Copy link
Contributor

jeysal commented May 12, 2019

In that case, this is actually the same issue as #2549
However, even though I knew about the realms problem, this surprises me, because I would have thought that rewire does not use require but instead reads the modules using fs or something, in which case I believe there would be no problem because both Objects would be from inside the VM

@georgezlei
Copy link
Author

Yes #2549 looks like the same issue.

I changed the code again and removed the rewire dependency. Now it looks like below
test.js

const vm = require('vm')
const code = `console.log(require("./rewired").there instanceof Object);`;
eval(code)
vm.runInNewContext(code, {require, console})
vm.runInNewContext(code, {require, console, Object})

rewired.js

exports.there = {}

The result is

true
false
true

Obviously this issue has spreaded to the any code reference to the tested source file. This is inevitable in any kind of test. Issue will always happen when instanceof operation is applied to the arguments of the function tested.

If Node is not willing to change this behaviour, I believe Jest need to do something here. Either discard the way of starting new vm context, or allow the user to pass in the base objects when needed.

@georgezlei
Copy link
Author

Got confirmation from Node developer that objects should use the context of their definition, not execution. So they don't think this is a problem. But for Jest this is a problem.

@jeysal
Copy link
Contributor

jeysal commented May 15, 2019

Let's close this to keep any discussion centralized in #2549. It's a long standing issue and there's a lot of info and a bounty, but it's a complex topic that might not be properly fixed in the near future :|

@jeysal jeysal closed this as completed May 15, 2019
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants