Allow JSON imports in NodeJS 16.15

[queengooborg]

This PR is a quick fix that enables JSON module support by default in NodeJS v16.15 < v17 as well as v17.5 and up. The version number was selected based upon prior manual testing to determine when JSON module support was enabled by default.

This change was tested by attempting to run a script that uses a JSON import and performing the same tweak directly in node_modules.

[cspotcode]

Thanks, seems simple enough. Was this mentioned in the nodejs 16.15 changelog? Would be nice to keep a link to that changelog somewhere, in case questions arise in the future.

[queengooborg]

Looks like it is, yes! https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V16.md#other-notable-changes

[codecov]

Codecov Report

Merging #1792 (c98b685) into main (14323f9) will not change coverage.
The diff coverage is n/a.

Impacted Files	Coverage Δ
dist-raw/node-internal-modules-esm-get_format.js	89.18% <ø> (ø)

[cspotcode]

Awesome, thanks!

We run our tests against a bunch of node versions to test stuff like this, and we conditionally check the node version in the tests to figure out what we should test.

Here is the test for JSON imports, where we check the node version. Depending on the check, we either pass --experimental-json-modules or we don't.

ts-node/src/test/esm-loader.spec.ts

Lines 270 to 306 in 17aad13

	test.suite('supports import assertions', (test) => {
	test.runIf(nodeSupportsImportAssertions);
	test.suite('node >=17.5.0', (test) => {
	test.runIf(semver.gte(process.version, '17.5.0'));
	test('Can import JSON modules with appropriate 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.trim()).toBe(
	'A fuchsia car has 2 seats and the doors are open.\nDone!'
	);
	});
	});
	test.suite('node <17.5.0', (test) => {
	test.runIf(semver.lt(process.version, '17.5.0'));
	test('Can import JSON using the appropriate flag and assertion', async (t) => {
	const { err, stdout } = await exec(
	`${CMD_ESM_LOADER_WITHOUT_PROJECT} --experimental-json-modules ./importJson.ts`,
	{
	cwd: resolve(TEST_DIR, 'esm-import-assertions'),
	}
	);
	expect(err).toBe(null);
	expect(stdout.trim()).toBe(
	'A fuchsia car has 2 seats and the doors are open.\nDone!'
	);
	});
	});
	});

Can you also update that version check in the tests? Most of the version checks are declared as boolean exports from helpers.ts, like this:

ts-node/src/test/helpers.ts

Lines 60 to 78 in 17aad13

	//#region version checks
	export const nodeSupportsEsmHooks = semver.gte(process.version, '12.16.0');
	export const nodeSupportsSpawningChildProcess = semver.gte(
	process.version,
	'12.17.0'
	);
	export const nodeUsesNewHooksApi = semver.gte(process.version, '16.12.0');
	export const nodeSupportsImportAssertions = semver.gte(
	process.version,
	'17.1.0'
	);
	/** Supports tsconfig "extends" >= v3.2.0 */
	export const tsSupportsTsconfigInheritanceViaNodePackages = semver.gte(
	ts.version,
	'3.2.0'
	);
	/** Supports --showConfig: >= v3.2.0 */
	export const tsSupportsShowConfig = semver.gte(ts.version, '3.2.0');
	//#endregion

Can we move this version check into that file? Can be named something like export const nodeSupportUnflaggedJsonImports =

[cspotcode]

Looks like the nodeSupportsImportAssertions boolean should also be updated to include node >= 16.14.0

https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V16.md#2022-02-08-version-16140-gallium-lts-danielleadams

ts-node/src/test/helpers.ts

Lines 67 to 70 in 17aad13

	export const nodeSupportsImportAssertions = semver.gte(
	process.version,
	'17.1.0'
	);

[cspotcode]

Tests on node nightly are failing due to a node bug; you can safely ignore those test failures.

[queengooborg]

Test updates completed -- I opted for the variable name nodeSupportsImportAssertionsTypeJson to match the syntax (assert {type: 'json'}) and open and preemptively opening the door for any other import assertion properties and values!

[queengooborg]

Hey there! Just checking in to see if this needs anything more from my end, or if we're just waiting on review?

[cspotcode]

@queengooborg Merged and will go out in the next release. Thanks!