Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Aug 12, 2020
Merged

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

merged 4 commits into from
Aug 12, 2020

Conversation

concision
Copy link
Contributor

Fixes #1098

…, and other unexpected file extensions
@codecov
Copy link

codecov bot commented Aug 10, 2020

Codecov Report

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

Impacted file tree graph

@@            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
Copy link

coveralls commented Aug 10, 2020

Coverage Status

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

@cspotcode
Copy link
Collaborator

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 cspotcode self-requested a review August 11, 2020 18:33
Copy link
Collaborator

@cspotcode cspotcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a regression test; otherwise looks good.

@cspotcode cspotcode mentioned this pull request Aug 11, 2020
@concision
Copy link
Contributor Author

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.

@concision concision marked this pull request as draft August 12, 2020 00:13
@cspotcode
Copy link
Collaborator

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
Copy link
Contributor Author

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.

@concision concision marked this pull request as ready for review August 12, 2020 05:00
@cspotcode
Copy link
Collaborator

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 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ignored() bug: ESM loader will try to compile .cjs, .mjs, and other unexpected file extensions
3 participants