Addressing review feedback

Author: cspotcode

src/esm.ts

@@ -62,7 +62,11 @@ export interface NodeLoaderHooksAPI2 {
 export namespace NodeLoaderHooksAPI2 {
   export type ResolveHook = (
     specifier: string,
-    context: { parentURL: string },
+    context: {
+      conditions?: NodeImportConditions;
+      importAssertions?: NodeImportAssertions;
+      parentURL: string;
+    },
     defaultResolve: ResolveHook
   ) => Promise<{ url: string }>;
   export type LoadHook = (
@@ -86,9 +90,10 @@ export type NodeLoaderHooksFormat =
   | 'module'
   | 'wasm';
 
-export type NodeImportAssertions = {
-  type: 'json' | 'wasm';
-};
+export type NodeImportConditions = unknown;
+export interface NodeImportAssertions {
+  type?: 'json';
+}
 
 /** @internal */
 export function registerAndCreateEsmHooks(opts?: RegisterOptions) {
@@ -187,8 +192,8 @@ export function createEsmHooks(tsNodeService: Service) {
       const { source: rawSource } = await defaultLoad(
         url,
         {
+          ...context,
           format,
-          importAssertions: context.importAssertions,
         },
         defaultLoad
       );

src/test/esm-loader.spec.ts

@@ -5,17 +5,19 @@
 import { context } from './testlib';
 import semver = require('semver');
 import {
+  CMD_ESM_LOADER_WITHOUT_PROJECT,
   contextTsNodeUnderTest,
   EXPERIMENTAL_MODULES_FLAG,
   resetNodeEnvironment,
   TEST_DIR,
 } from './helpers';
 import { createExec } from './exec-helpers';
-import { join } from 'path';
+import { join, resolve } from 'path';
 import * as expect from 'expect';
 import type { NodeLoaderHooksAPI2 } from '../';
 
 const nodeUsesNewHooksApi = semver.gte(process.version, '16.12.0');
+const nodeSupportsImportAssertions = semver.gte(process.version, '17.1.0');
 
 const test = context(contextTsNodeUnderTest);
 
@@ -71,3 +73,55 @@ test.suite('hooks', (_test) => {
     });
   }
 });
+
+if (nodeSupportsImportAssertions) {
+  test.suite('Supports import assertions', (test) => {
+    test('Can import JSON using the appropriate flag and assertion', async (t) => {
+      const { err, stdout } = await exec(
+        `${CMD_ESM_LOADER_WITHOUT_PROJECT} ./importJson.ts`,
+        {
+          cwd: resolve(TEST_DIR, 'esm-import-assertions'),
+        }
+      );
+      expect(err).toBe(null);
+      expect(stdout).toMatch(
+        'A fuchsia car has 2 seats and the doors are open.`\nDone!'
+      );
+    });
+  });
+
+  test.suite("Catch unexpected changes to node's loader context", (test) => {
+    /*
+     * This does not test ts-node.
+     * Rather, it is meant to alert us to potentially breaking changes in node's
+     * loader API.  If node starts returning more or less properties on `context`
+     * objects, we want to know, because it may indicate that our loader code
+     * should be updated to accomodate the new properties, either by proxying them,
+     * modifying them, or suppressing them.
+     */
+    test('Ensure context passed to loader by node has only expected properties', async (t) => {
+      const { stdout, stderr } = await exec(
+        `node --loader ./esm-loader-context/loader.mjs --experimental-json-modules ./esm-loader-context/index.mjs`
+      );
+      const rows = stdout.split('\n').filter((v) => v[0] === '{');
+      expect(rows.length).toBe(14);
+      rows.forEach((row) => {
+        const json = JSON.parse(row) as {
+          resolveContextKeys?: string[];
+          loadContextKeys?: string;
+        };
+        if (json.resolveContextKeys) {
+          expect(json.resolveContextKeys).toEqual([
+            'conditions',
+            'importAssertions',
+            'parentURL',
+          ]);
+        } else if (json.loadContextKeys) {
+          expect(json.loadContextKeys).toEqual(['format', 'importAssertions']);
+        } else {
+          throw new Error('Unexpected stdout in test.');
+        }
+      });
+    });
+  });
+}

tests/esm-import-assertions/importJson.ts

@@ -12,3 +12,16 @@ if (dynamicCarData.doors !== 'open')
 console.log(
   `A ${carData.color} car has ${carData.seats} seats and the doors are ${dynamicCarData.doors}.`
 );
+
+// Test that omitting the assertion causes node to throw an error
+await import('./car.json')
+  .then(() => {
+    throw new Error('should have thrown');
+  },
+  (error: Error) => {
+    if(!error.message.includes('foo bar')) {
+      throw error;
+    }
+    /* error is expected */
+  });
+console.log('Done!');

tests/esm-loader-context/index.mjs

@@ -0,0 +1,7 @@
+import * as moduleA from './moduleA.mjs';
+import * as moduleB from './moduleB.mjs' assert { foo: 'bar' };
+import * as jsonModule from './jsonModuleA.json' assert { type: 'json' };
+
+await import('./moduleC.mjs');
+await import('./moduleD.mjs', { foo: 'bar' });
+await import('./jsonModuleB.json', { assert: { type: 'json' } });

tests/esm-loader-context/jsonModuleA.json

@@ -0,0 +1 @@
+{}

tests/esm-loader-context/jsonModuleB.json

@@ -0,0 +1 @@
+{}

tests/esm-loader-context/loader.mjs

@@ -0,0 +1,8 @@
+export function resolve(specifier, context, defaultResolve) {
+  console.log(JSON.stringify({ resolveContextKeys: Object.keys(context) }));
+  return defaultResolve(specifier, context);
+}
+export function load(url, context, defaultLoad) {
+  console.log(JSON.stringify({ loadContextKeys: Object.keys(context) }));
+  return defaultLoad(url, context);
+}