Merge pull request #100 from cardstack/hassan/cs-141-error-catalogjs-…

…runtime-error-nodejs

Fixes CS-141: add support for providing polyfills for CJS files

packages/builder-node/src/nodes/cjs-interop.ts

@@ -29,6 +29,20 @@ import {
 import { PackageSrcNode, buildSrcDir } from "./package";
 // @ts-ignore repl.builtinModules is a new API added in node 14.5.0, looks like TS lint has not caught up
 import { builtinModules } from "repl";
+import difference from "lodash/difference";
+
+const polyfills = [
+  "assert",
+  "buffer",
+  "process",
+  "util",
+  "path",
+  "url",
+  "querystring",
+  "punycode",
+  // "fs", our fs is just a shim. we should not try to polyfill this, and let it throw if this is actually evaluated.
+  "os",
+];
 
 export class MakePkgESCompliantNode implements BuilderNode {
   cacheKey: string;
@@ -258,17 +272,23 @@ class RewriteCJSNode implements BuilderNode {
   async deps() {}
 
   async run(): Promise<NodeOutput<void[]>> {
+    let nonPolyfilledBuiltins = difference(builtinModules, polyfills);
     let jsonSpecifiers = new Set<string>();
     let imports = new Set<string>(
       this.desc.requires.map(({ specifier }) => {
         if (specifier == null) {
           return `import { requireHasNonStringLiteralSpecifier } from "@catalogjs/loader";`;
         }
         let pkgInfo = pkgInfoFromSpecifier(specifier);
-        // TODO polyfill what we can...
-        if (builtinModules.includes(pkgInfo?.pkgName)) {
+        if (
+          pkgInfo?.pkgName &&
+          nonPolyfilledBuiltins.includes(pkgInfo.pkgName)
+        ) {
           return `import { requireNodeBuiltin } from "@catalogjs/loader";`;
         }
+        if (pkgInfo?.pkgName && polyfills.includes(pkgInfo.pkgName)) {
+          return `import ${pkgInfo.pkgName} from "@catalogjs/polyfills/${pkgInfo.pkgName}";`;
+        }
         if (isLocalJSON(specifier)) {
           jsonSpecifiers.add(specifier); // side-effecty, but we're right here anyways...
           return `import ${depJSONName(specifier)} from "${specifier}.js";`;
@@ -291,8 +311,11 @@ class RewriteCJSNode implements BuilderNode {
         })`;
       }
       let pkgInfo = pkgInfoFromSpecifier(specifier);
-      if (builtinModules.includes(pkgInfo?.pkgName)) {
-        return `requireNodeBuiltin("${pkgInfo!.pkgName}")`;
+      if (pkgInfo?.pkgName && nonPolyfilledBuiltins.includes(pkgInfo.pkgName)) {
+        return `requireNodeBuiltin("${pkgInfo.pkgName}")`;
+      }
+      if (pkgInfo?.pkgName && polyfills.includes(pkgInfo.pkgName)) {
+        return `() => ${pkgInfo.pkgName}`;
       }
       if (isLocalJSON(specifier)) {
         return `get${upperFirst(depJSONName(specifier))}`;

packages/builder-node/src/nodes/package.ts

@@ -159,9 +159,11 @@ class FinishPackagePreparationNode implements BuilderNode {
         esCompliance,
         // all node builds have the runtime loader package available as a
         // dependency, so that runtime loading situations (e.g. a dynamic
-        // require specifier) can be handled if they arise
+        // require specifier) can be handled if they arise, as well as a
+        // polyfills package to polyfill node-isms
         {
           "@catalogjs/loader": "^0.0.1",
+          "@catalogjs/polyfills": "^0.0.1",
         }
       ),
       esCompliance,

packages/builder-node/test/install-test.ts

@@ -138,6 +138,11 @@ QUnit.module("Install from npm", function () {
           pkgName: "@catalogjs/loader",
           range: "^0.0.1",
         },
+        "@catalogjs/polyfills": {
+          type: "npm",
+          pkgName: "@catalogjs/polyfills",
+          range: "^0.0.1",
+        },
       });
 
       let cEntrypoints = JSON.parse(
@@ -154,6 +159,11 @@ QUnit.module("Install from npm", function () {
           pkgName: "@catalogjs/loader",
           range: "^0.0.1",
         },
+        "@catalogjs/polyfills": {
+          type: "npm",
+          pkgName: "@catalogjs/polyfills",
+          range: "^0.0.1",
+        },
       });
     });
 
@@ -202,6 +212,11 @@ QUnit.module("Install from npm", function () {
           pkgName: "@catalogjs/loader",
           range: "^0.0.1",
         },
+        "@catalogjs/polyfills": {
+          type: "npm",
+          pkgName: "@catalogjs/polyfills",
+          range: "^0.0.1",
+        },
       });
 
       let { e: pkgEURL } = aLock;
@@ -218,6 +233,11 @@ QUnit.module("Install from npm", function () {
           pkgName: "@catalogjs/loader",
           range: "^0.0.1",
         },
+        "@catalogjs/polyfills": {
+          type: "npm",
+          pkgName: "@catalogjs/polyfills",
+          range: "^0.0.1",
+        },
       });
       let eLock = JSON.parse(
         await (await fs.openFile(new URL("catalogjs.lock", pkgEURL))).readText()
@@ -250,6 +270,11 @@ QUnit.module("Install from npm", function () {
           pkgName: "@catalogjs/loader",
           range: "^0.0.1",
         },
+        "@catalogjs/polyfills": {
+          type: "npm",
+          pkgName: "@catalogjs/polyfills",
+          range: "^0.0.1",
+        },
       });
       assert.notOk(d1Entrypoints.html);
       let dLock = JSON.parse(
@@ -280,6 +305,11 @@ QUnit.module("Install from npm", function () {
           pkgName: "@catalogjs/loader",
           range: "^0.0.1",
         },
+        "@catalogjs/polyfills": {
+          type: "npm",
+          pkgName: "@catalogjs/polyfills",
+          range: "^0.0.1",
+        },
       });
 
       let b2Lock = JSON.parse(
@@ -362,16 +392,21 @@ QUnit.module("Install from npm", function () {
             require(file);
           }`,
         "e.js": `
-          const fs = require('fs');
-          module.exports.nope = function(filename) {
-            return fs.readFileSync(filename);
+          const tty = require('tty');
+          module.exports.nope = function() {
+            return tty.something();
           }`,
         "f.js": `
           const sample = require('./sample.json');
           module.exports.foo = sample.foo;`,
         "sample.json": `{
           "foo": "bar"
         }`,
+        "g.js": `
+          const assert = require('assert');
+          module.exports.ok = function(value) {
+            assert(value, "value is truthy");
+          }`,
         "es-module.js": `
           import { foo } from "./sample2.json";
           export { bleep } from "./sample3.json";
@@ -607,7 +642,7 @@ module.exports.default = dependencies;\`
       );
     });
 
-    test("it includes a special runtime loader for require() of node builtin module", async function (assert) {
+    test("it includes a special runtime loader for require() of of non-polyfilled nodejs builtin module", async function (assert) {
       let [testPkgURL] = packageURLs;
       let src = await (
         await fs.openFile(new URL(`${buildSrcDir}e.cjs.js`, testPkgURL))
@@ -624,11 +659,40 @@ module.exports.default = dependencies;\`
               "module",
               "exports",
               "dependencies",
-              \`const fs = dependencies[0]();
-module.exports.nope = function (filename) {
-  return fs.readFileSync(filename);
+              \`const tty = dependencies[0]();
+module.exports.nope = function () {
+  return tty.something();
+};\`
+            )(module, module.exports, [requireNodeBuiltin("tty")]);
+          }
+          return module.exports;
+        }
+        export { implementation as default, implementation as cjs};`
+      );
+    });
+
+    test("it polyfills node builtin module when we have polyfill available", async function (assert) {
+      let [testPkgURL] = packageURLs;
+      let src = await (
+        await fs.openFile(new URL(`${buildSrcDir}g.cjs.js`, testPkgURL))
+      ).readText();
+      assert.codeEqual(
+        src,
+        `
+        import assert from "@catalogjs/polyfills/assert";
+        let module;
+        function implementation() {
+          if (!module) {
+            module = { exports: {} };
+            Function(
+              "module",
+              "exports",
+              "dependencies",
+              \`const assert = dependencies[0]();
+module.exports.ok = function (value) {
+  assert(value, "value is truthy");
 };\`
-            )(module, module.exports, [requireNodeBuiltin("fs")]);
+            )(module, module.exports, [() => assert]);
           }
           return module.exports;
         }

packages/builder-worker/package.json

@@ -39,13 +39,15 @@
     "@msgpack/msgpack": "^1.12.2",
     "@types/babel__core": "^7.1.7",
     "@types/babel__traverse": "^7.0.10",
+    "@types/diff": "^5.0.0",
     "@types/imurmurhash": "^0.1.1",
     "@types/json-stable-stringify": "^1.0.32",
     "@types/lodash": "^4.14.150",
     "@types/semver": "^7.3.1",
     "base64-arraybuffer": "^0.2.0",
     "bind-decorator": "^1.0.11",
     "columnify": "^1.5.4",
+    "diff": "^5.0.0",
     "dom-serializer": "^0.2.2",
     "fast-stable-stringify": "^1.0.0",
     "htmlparser2": "^4.1.0",

packages/builder-worker/test/helpers/code-equality-assertions.ts

@@ -1,6 +1,7 @@
 import { transform } from "@babel/core";
 import { NodePath } from "@babel/traverse";
 import { StringLiteral } from "@babel/types";
+import { createPatch } from "diff";
 
 declare global {
   interface Assert {
@@ -32,11 +33,23 @@ function codeEqual(
   expected: string,
   message?: string
 ) {
+  let parsedActual = standardize(actual)!;
+  let parsedExpected = standardize(expected)!;
+  let msg: string = "code is not equal.";
+  //@ts-ignore this is just a check to see if we are in nodejs
+  if (typeof window === "undefined") {
+    msg = `${msg}
+${createPatch("", parsedExpected, parsedActual)
+  .split("\n")
+  .slice(4)
+  .join("\n")}`;
+  }
+
   this.pushResult({
-    result: standardize(actual) === standardize(expected),
-    actual: actual,
-    expected: expected,
-    message: message ?? "source code comparison",
+    result: parsedActual === parsedExpected,
+    actual,
+    expected,
+    message: message ?? msg,
   });
 }
 

packages/polyfills/buffer.js

@@ -2143,3 +2143,11 @@ export {
   isBuffer,
   _kMaxLength as kMaxLength,
 };
+
+export default {
+  Buffer,
+  INSPECT_MAX_BYTES,
+  SlowBuffer,
+  isBuffer,
+  kMaxLength: _kMaxLength,
+};

packages/polyfills/entrypoints.json

@@ -10,6 +10,7 @@
     "./url.js",
     "./querystring.js",
     "./punycode.js",
-    "./fs-stub.js"
+    "./fs.js",
+    "./os.js"
   ]
 }

packages/polyfills/index.js

@@ -6,4 +6,5 @@ export { default as path } from "./path";
 export { default as url } from "./url";
 export { default as querystring } from "./querystring";
 export { default as punycode } from "./punycode";
-export { default as fs } from "./fs-stub";
+export { default as fs } from "./fs";
+export { default as os } from "./os";