Fix #1098: ignored() bug: ESM loader will try to compile .cjs, .mjs…

[concision]

Fixes #1098

[codecov]

Codecov Report

Merging #1103 into master will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1103      +/-   ##
==========================================
+ Coverage   75.49%   75.68%   +0.18%     
==========================================
  Files           7        7              
  Lines         657      658       +1     
  Branches      148      147       -1     
==========================================
+ Hits          496      498       +2     
  Misses        107      107              
+ Partials       54       53       -1     

Flag	Coverage Δ
#node_10	72.40% <100.00%> (+0.24%)	⬆️
#node_12_15	72.77% <100.00%> (+0.24%)	⬆️
#node_12_16	72.77% <100.00%> (+0.24%)	⬆️
#node_13	75.37% <100.00%> (+0.18%)	⬆️
#node_14	75.37% <100.00%> (+0.18%)	⬆️
#typescript_2_7	75.22% <100.00%> (+0.18%)	⬆️
#typescript_latest	74.46% <100.00%> (+0.19%)	⬆️
#typescript_next	74.31% <100.00%> (+0.19%)	⬆️
#ubuntu	75.37% <100.00%> (+0.18%)	⬆️
#windows	75.53% <100.00%> (+0.18%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/esm.ts	68.88% <100.00%> (ø)
src/index.ts	78.18% <100.00%> (+0.34%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce7c323...072519d. Read the comment docs.

[coveralls]

Coverage increased (+1.0%) to 83.201% when pulling 072519d on concision:ab/1098 into ce7c323 on TypeStrong:master.

[cspotcode]

Looks good; can we add a test for this? We might be able to add to one of our existing ESM test cases. We need to make sure that the test will fail if ts-node attempts to compile a .cjs or an .mjs file.

[cspotcode]

Needs a regression test; otherwise looks good.

[concision]

I have added a few white-box tests for ignored() to check that the correct extensions are included/excluded, based on allowJs and jsx. Notably, all combinations of allowJs and jsx should ignore other extensions such as .cjs, .mjs, .xyz, and even extension-less files. I am not sure if you had anything more elaborate in mind, but I think this covers tests for the scope of this issue.

---

I did have a question regarding mixed capitalization extensions (e.g. .tS, .Ts, .TS). The TypeScript compiler seems to accept these mixed capitalization extensions, but ts-node does not. Should we support sending these unconventional extensions to the TypeScript compiler (i.e. when ignored("filename.TS") === false)?

This could be accomplished with a .toLowerCase() call on ext here (and in conjunction with the change in the next section):

ts-node/src/index.ts

Lines 837 to 846 in de289f6

	const extensions = getExtensions(config)
	const ignored = (fileName: string) => {
	if (!active) return true
	const ext = extname(fileName)
	if (extensions.tsExtensions.includes(ext) || extensions.jsExtensions.includes(ext)) {
	const relname = relative(cwd, fileName)
	return !isScoped(relname) || shouldIgnore(relname)
	}
	return true
	}

---

I also think the ESM loader getFormat() function should be amended to use the ignore function for consistency:

ts-node/src/esm.ts

Lines 73 to 77 in 77f568d

	// If file has .ts, .tsx, or .jsx extension, then ask node how it would treat this file if it were .js
	const ext = extname(nativePath)
	if (ext === '.ts' || ext === '.tsx' || ext === '.jsx') {
	return defer(formatUrl(pathToFileURL(nativePath + '.js')))
	}

Instead:

    // If file is not ignored, then ask node how it would treat this file if it were .js
    if (!tsNodeInstance.ignored(nativePath)) {
      return defer(formatUrl(pathToFileURL(nativePath + '.js')))
    }

Although, I am not sure if there are any ramifications to this. Let me know what you think.

[cspotcode]

Tests look great.

Should we add the .d.ts extension to the tests? Thinking out loud, if node's module resolution points to a .d.ts file -- either in CommonJS or with our ESM hooks -- then something has gone wrong, but it's probably not our fault. In that case, we do not want to ignore that file, because surely node cannot handle it. If we handle the .d.ts file, then we'll be able to throw a meaningful error message.

Agreed about amending the getFormat hook; good catch. Should we combine ignored() with the existing ext === checks? There's no need to do nativePath + '.js' if the file extension is already .js.

Regarding mixed capitalization extensions: I just tested, and our require hooks don't support mixed capitalization on Linux. Seems like this has been a very long-standing limitation and we haven't received any bug reports about it. I feel confident leaving it out of this pull request. If you want, feel free to file a separate issue to investigate. But I don't think it's a high priority.

[concision]

Currently ignored("xyz.d.ts") === false, so .d.ts files will be blindly accepted and piped to the TypeScript compiler. I was curious what happens if I try to coerce the loader to run a .d.ts directly. Looks like we have been beaten by a few months already, as this was the error that was raised:

ts-node/src/index.ts

Lines 629 to 637 in 77f568d

	// Throw an error when requiring `.d.ts` files.
	if (output.outputFiles.length === 0) {
	throw new TypeError(
	`Unable to require file: ${relative(cwd, fileName)}\n` +
	'This is usually the result of a faulty configuration or import. ' +
	'Make sure there is a `.js`, `.json` or other executable extension with ' +
	'loader attached before `ts-node` available.'
	)
	}

I think it is fine as is, this error will be automatically raised if somehow a user manages to get a .d.ts file to be interpreted as a module. Good to go on this front, I think.

I have made the getFormat change to the following. I think this should be sufficient.

ts-node/src/esm.ts

Lines 73 to 77 in afdf964

	// If file has .ts, .tsx, or .jsx extension, then ask node how it would treat this file if it were .js
	const ext = extname(nativePath)
	if (ext !== '.js' && !tsNodeInstance.ignored(nativePath)) {
	return defer(formatUrl(pathToFileURL(nativePath + '.js')))
	}

I am mildly surprised that no issue has ever been raised about mixed capitalized extensions, but it definitely is not a priority or really that important. A nitpick, I suppose. Thanks for looking into it.

[cspotcode]

Looks great. I added .d.ts extensions to the tests to avoid any surprises there, and tests are still passing so I'll go ahead and merge this.

Thanks again @concision !