bug: assertEquals is not transpiled correctly

[KnorpelSenf]

Description

Node implements __proto__ and Deno does not. assertEquals relies on comparing string representations so it fails on Node but works on Deno.

Repro

1. Take this file

import { assertEquals } from "https://deno.land/std@0.166.0/testing/asserts.ts";

let p = "__proto__";
let o = {};
(o as any)[p] = 0;
assertEquals({ ["__proto__"]: 0 }, o);

2. Use the build script from the README

// ex. scripts/build_npm.ts
import { build, emptyDir } from "https://deno.land/x/dnt/mod.ts";

await emptyDir("./npm");

await build({
  entryPoints: ["./mod.ts"],
  outDir: "./npm",
  shims: {
    // see JS docs for overview and more options
    deno: true,
  },
  package: {
    // package.json properties
    name: "your-package",
    version: Deno.args[0],
    description: "Your package.",
    license: "MIT",
    repository: {
      type: "git",
      url: "git+https://github.com/username/repo.git",
    },
    bugs: {
      url: "https://github.com/username/repo/issues",
    },
  },
});

3. Run deno run mod.ts and see how it works
4. Run node npm and see how it throws:

$ node npm
/tmp/test/npm/script/deps/deno.land/std@0.166.0/testing/asserts.js:178
    throw new AssertionError(message);
    ^

AssertionError: Values are not equal:


    [Diff] Actual / Expected


-   {
-     ['__proto__']: 0
-   }
+   {}

    at assertEquals (/tmp/test/npm/script/deps/deno.land/std@0.166.0/testing/asserts.js:178:11)
    at Object.<anonymous> (/tmp/test/npm/script/mod.js:7:31)
    at Module._compile (node:internal/modules/cjs/loader:1119:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1173:10)
    at Module.load (node:internal/modules/cjs/loader:997:32)
    at Module._load (node:internal/modules/cjs/loader:838:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:18:47

Node.js v18.9.0

[dsherret]

I don't think there's anything dnt can do here. This should be handled in deno_std I think.

[KnorpelSenf]

Feel free to transfer the issue in that case. I wasn't aware that /std has the aim to be compatible with Node environments.

[dsherret]

Opened denoland/std#2954

[schwarzkopfb]

@dsherret as far as I understand, dnt's purpose is to make Deno-first apps runnable inside Node.js. Therefore I think the goal here is to mimic Deno's aspects in the Node.js runtime as close as possible. The error in question is that Node.js is still supporting the ancient __proto__ special property, which is excluded from Deno by decision.

We should prepend the following line to the generated output (or even add it into somewhere shim-deno):

delete Object.prototype.__proto__;

This is exactly how Deno disables __proto__, so it'll guarantee the same behaviour in Node.js.

I tested this idea while researching this issue and it works. For me it looks like a much better approach than modifying std for the needs of this specific case.

WDYT?

[dsherret]

We should prepend the following line to the generated output

That could break other npm packages that use __proto__. I don't think we should do it. __proto__ is changing in Deno too: denoland/deno#16775

[schwarzkopfb]

1. This tool ports apps which are working with Deno into Node.js. npm packages relying on __proto__ are not running in Deno anyway. Sorry for my probably dumb comment, I'm just wondering what kind of compatibility do we loose in the context of dnt. 🤔 (Seems like I lack the proper knowledge here. 😅)

2. We can prepend the new setter introduced in the PR you linked instead of just deleting the property entirely to stay in line with Deno.

[KnorpelSenf]

Ideally, Deno would support __proto__ inside modules imported through npm:, but nowhere else. Does it work that way?

Ideally, dnt resembles this behaviour on Node. This would effectively mean that __proto__ isn't available anywhere, except for the modules imported from npm. Is this even feasible?

[kt3k]

Does this difference actually affect the module testing? If there's no actual example usage, then maybe we can keep this behavior as a known limitation

[KnorpelSenf]

Yes. That's why I opened this issue.

I use property-based testing which generates arbitrary objects that could be returned from JSON.parse. I expect my module to be able to handle these objects. All tests pass on Deno, but the build fails because the same tests don't work on Node.

I currently implemented a fairly dirty workaround (it's at https://github.com/KnorpelSenf/oson/blob/ab4bb0e7081f0593acf5e85562fbce1d685ad2ee/mod.test.ts#L13 but please don't look at it) to skip calling assertEquals when the tests are running in a Node env and there are __proto__ keys.

[kt3k]

How about passing --disable-proto=delete option to node? I guess that might be a better workaround https://nodejs.org/api/cli.html#--disable-protomode

[KnorpelSenf]

It would be great if dnt would do this when running the tests, yes. Is there a way to configure it?

[kt3k]

How about passing via NODE_OPTIONS env var, like:

NODE_OPTIONS='--disable-proto=delete' deno run -A scripts/build_npm.ts

[KnorpelSenf]

Well fair enough, that makes sense, thanks