node

node copied to clipboard

• Version: v12.6.0 (also seen in 10.13.0)
• Platform: Darwin Davids-MacBook-Pro.local 18.5.0 Darwin Kernel Version 18.5.0: Mon Mar 11 20:40:32 PDT 2019; root:xnu-4903.251.3~3/RELEASE_X86_64 x86_64
• Subsystem:

I've created a simple testing framework that runs tests using vm.Script.runInContext(). Now I'm writing tests for code that uses the whatwg URL API. If I use vm.createContext(), the created context does not define the URL() constructor. But if I pass in the URL constructor with vm.createContext({URL}), then I have a situation where arrays returned by URLSearchParams methods are defined using the Array.prototype object from outside the context, and my tests are trying to compare those to arrays defined inside the context with a different Array.prototype object. So because I have two arrays with different prototypes, assert.deepStrictEqual() thinks they are not the same.

I'd argue that the underlying bug here is that URL should be automatically defined in newly created contexts without having to be passed in. Or maybe this is a bug in assert.deepStrictEqual() and it is stricter than it ought to be in this cross-context situation?

In any case, here is an example that reproduces the issue for me:

const vm = require('vm');

// URL is not defined inside the context, and I can't require it, so
// I need to pass it to the context from outside. But it returns arrays
// using the Array class from outside the context.
let context = vm.createContext({require, URL, externalArray:Array});

let script = new vm.Script(`
    const assert = require('assert');
    let url = new URL('http://example.com');
    url.searchParams.append('x', '1');
    url.searchParams.append('x', '2');
    let actual = url.searchParams.getAll('x'); // Uses array class from outside
    let expected = ['1', '2'];                 // Uses array class from inside
    assert(Array.isArray(actual));                                // passes
    assert.deepStrictEqual(Array.from(actual), expected);         // passes
    assert.deepStrictEqual(actual, externalArray.from(expected)); // passes
    assert.deepStrictEqual([...actual], expected);                // passes
    assert.deepStrictEqual(actual, expected);               // fails
    assert.equal(Object.getPrototypeOf(actual),             // also fails
                 Object.getPrototypeOf(expected)); 
`);

script.runInContext(context);

new contexts don't contain anything node.js-specific (Buffer, URL, process, etc). Also you can require it, it's require('url').URL.

I’ve labelled this feature request because, while this is currently expected behaviour, I can see that it makes sense to provide some/most/all Node.js features for multiple contexts in some way.

Also you can require it, it's require('url').URL.

That doesn’t yield an object in the Node.js main context, though, so it’s probably not quite as useful in a different vm Context.

vm.createNodeContext() or something might be interesting.

That doesn’t yield an object in the Node.js main context, though, so it’s probably not quite as useful in a different vm Context.

Oh I didn't mean to suggest that require('url').URL was a solution, I was just responding to URL is not defined inside the context, and I can't require it,

Thanks for the require('url').URL tip (the docs are unclear on that...)

Surpisingly, even when URL is required into the context that way, the URL API still ends up returning arrays that are not compatible with array literals created in the context. Here's my modified test case that still fails in the same way. Is this still "currently expected behavior"? I suppose that since I'm calling a require() passed in from outside, maybe it is requiring the same outside version of URL().

Is it expected behavior that assert.deepStrictEqual() would fail to compare arrays defined in two different contexts like this? Or is there a legitimate argument to be made that this is a bug in the assert module? Here's the updated test case that uses require('url').URL but still fails

const vm = require('vm');

// URL is not defined inside the context, and I can't require it, so
// I need to pass it to the context from outside. But it returns arrays
// using the Array class from outside the context.
let context = vm.createContext({require, externalArray:Array});

let script = new vm.Script(`
    const assert = require('assert');
    const URL = require('url').URL;
    let url = new URL('http://example.com');
    url.searchParams.append('x', '1');
    url.searchParams.append('x', '2');
    let actual = url.searchParams.getAll('x'); // Uses array class from outside
    let expected = ['1', '2'];                 // Uses array class from inside
    assert(Array.isArray(actual));                                // passes
    assert.deepStrictEqual(Array.from(actual), expected);         // passes
    assert.deepStrictEqual(actual, externalArray.from(expected)); // passes
    assert.deepStrictEqual([...actual], expected);                // passes
    assert.deepStrictEqual(actual, expected);               // fails
    assert.equal(Object.getPrototypeOf(actual),             // also fails
                 Object.getPrototypeOf(expected)); 
`);

script.runInContext(context);

Is it expected behavior that assert.deepStrictEqual() would fail to compare arrays defined in two different contexts like this?

Yes. Node.js considers the “strict” in “deep strict equal” to mean that the objects have the same prototype (at least in recent versions), and that’s not the case for objects whose prototypes are from different contexts.

This may be unintuitive for built-in types like plain objects and arrays, but it makes sense once you think of it as comparing instances of two different but identical-looking classes (e.g. assert.deepStrictEqual(new (class A {}), new (class A {})) fails too, because the objects are of different classes).

And I see that the prototype comparison with === is clearly documented at https://nodejs.org/api/assert.html#assert_comparison_details_1, so modifying deepStrictEqual() would probably be a breaking change.

I would expect assert.deepStrictEqual(new (class A {}), new (class A {})) to fail because those are clearly different classes with the same name. But it would be nice if there was a deep equality check that sidestepped this cross-context problem. I wonder how Jest has dealt with deep equality, since I gather that they also run tests in separate contexts...

Thanks again for the quick responses. I guess I agree that this is a feature request and not actually a bug.

But it would be nice if there was a deep equality check that sidestepped this cross-context problem.

@davidflanagan I agree that that would be nice, but in the end the problem is that there’s no real difference between objects from different contexts and objects with different but structurally equivalent classes from the same context.

So, yes, I think all that we can do about this particular issue would be considering to expose the URL constructor and/or other Node.js builtin features for multiple contexts.

@rosaxny and I would be working on this. Thanks!

For JSDOM, we don’t want any node‑specific things (e.g.: Buffer, process, global, etc.) to be added to brand‑new contexts by default.

@ryzokuken @rosaxny any news? Being able to add Node's "extra" globals into a vm.Context without breaking instanceof would be lovely and fix some very confusing bugs in Jest.

With Node 15 adding a few more globals (Event, EventTarget, AbortController etc.) this is problem is more and more likely to hit consumers. Any chance of some movement here that's not reported? 😀

I've added some code to Jest to try to inspect the current env and copy over globals (https://github.com/facebook/jest/blob/49393d01cdda7dfe75718aa1a6586210fa197c72/packages/jest-environment-node/src/index.ts#L74-L103, not sure if the semantics using Object.defineProperty are correct), but I'd still love to see a separate createNodeVmContext or some such instead of manually copying over.

Duplicating my comments in https://github.com/nodejs/node/issues/31852#issuecomment-1247578125, we are getting close to get this done now with the progress in the ShadowRealms integration (a good chunk of underlying work is being done, though we need to do additional churn for side-effect-ful builtins, fortunately URL is not one of them and is available in ShadowRealms so that makes it more prioritized)

Seems to me the last comment before the bot indicates something like this is still planned, so I don't think it's stale 🙂

Maybe this can get the never-stale label?

FWIW, node:repl has a reasonable workaround for this issue, copying properties over (with their descriptors) from the calling globalThis that don't already exist.