Test coverage: moduleType overrides during ts-node loader usage #1376

jayaddison · 2021-06-20T19:35:34Z

Extends #1371.

This isn't working as expected currently: when running the test.esm.js module directly (from the base directory: TS_NODE_PROJECT=tests/module-types/tsconfig.json node --trace-warnings --experimental-specifier-resolution=node --loader ts-node/esm tests/module-types/test.esm.js), one of the assertions fails because both imported modules have a value of function for requireType.

Without the --experimental-specifier-resolution=node flag, a ERR_UNSUPPORTED_DIR_IMPORT error is raised by the attempts to import modules from directory paths.

… applied for ts-node in loader mode

cspotcode · 2021-06-23T15:47:05Z

Thanks, what needs to happen next? Does tests/module-types/tsconfig.json need to be modified to add the necessary rule for esm-exception?

Without the --experimental-specifier-resolution=node flag, a ERR_UNSUPPORTED_DIR_IMPORT error is raised by the attempts to import modules from directory paths.

Is this necessary to get code coverage?

codecov · 2021-06-23T15:49:24Z

Codecov Report

Merging #1376 (c0a954f) into ab/override-module-type (dcbd916) will increase coverage by 0.32%.
The diff coverage is n/a.

Impacted Files	Coverage Δ
src/esm.ts	`87.50% <0.00%> (+7.14%)`	⬆️

jayaddison · 2021-06-23T19:20:40Z

Does tests/module-types/tsconfig.json need to be modified to add the necessary rule for esm-exception?

I don't think so - that tsconfig.json should already set an ESM override for files in subdirectores, if I understand it correctly?

(just in case, I did try adding an explicit esm-exceptions subdirectory configuration there; no dice, both requireType values are still returned as function)

Without the --experimental-specifier-resolution=node flag, a ERR_UNSUPPORTED_DIR_IMPORT error is raised by the attempts to import modules from directory paths.

Is this necessary to get code coverage?

Not for code coverage, no, but to get the test module to run without raising that error it does seem to be required.

cspotcode · 2021-06-23T19:28:06Z

I don't think so - that tsconfig.json should already set an ESM override for files in subdirectores, if I understand it correctly?

Oh, I see the confusion. The logic shares elements of both TS "include" globbing and .gitignore / .gitattributes.

Globs match the behavior of TS "include", so src/cjs-subdir matches everything in that directory. It is an "implicit glob."

Patterns are matched from top to bottom, and anything at the bottom overrides anything at the top. Same as .gitignore or .gitattributes where anything at the bottom overrides anything at the top.

ts-node/tests/module-types/tsconfig.json

Lines 6 to 8 in c9c443c

    
           // Test that subsequent patterns override earlier ones 
        
           "src/cjs-subdir/**/*": "esm", 
        
           "src/cjs-subdir": "cjs"

Not for code coverage, no, but to get the test module to run without raising that error it does seem to be required.

What about importing the files by name instead of importing the directory?

jayaddison · 2021-06-23T20:15:24Z

Thank you - I've applied some rearrangements here, and am now able to run TS_NODE_PROJECT=tests/module-types/tsconfig.json node --trace-warnings --experimental-specifier-resolution=node --loader ts-node/esm tests/module-types/test.esm.js and get a passing result.

That said I'll want to reconfirm that the test coverage is really proving what it claims to, and also ensure that it works when run as part of the standard test suite.

jayaddison · 2021-06-23T20:18:13Z

(to try to explain what I meant about proving the test coverage: we should get confidence that the overrides are really being used, to make sure that passing test results aren't simply a result of default behaviour)

…dule override test

cspotcode · 2021-06-23T20:59:54Z

to make sure that passing test results aren't simply a result of default behaviour

Agreed. Does it make sense to copy-paste tests/module-types into tests/module-types-2, and make it into an opposite of the first project? That is to say, one of them has package.json type: "module" and uses overrides to force CommonJS, and the other project has package.json without type and uses overrides to force ESM.

Can we remove --experimental-specifier-resolution=node? That would make the tests a bit simpler to understand.

jayaddison · 2021-06-23T21:36:44Z

to make sure that passing test results aren't simply a result of default behaviour

Agreed. Does it make sense to copy-paste tests/module-types into tests/module-types-2, and make it into an opposite of the first project? That is to say, one of them has package.json type: "module" and uses overrides to force CommonJS, and the other project has package.json without type and uses overrides to force ESM.

Sounds good; I've been messing around a bit further here as you'll see, but the tests do not pass yet.

Can we remove --experimental-specifier-resolution=node? That would make the tests a bit simpler to understand.

~~I'll try to, yep. Whenever I've tried that previously it has led to the ERR_UNSUPPORTED_DIR_IMPORT errors, but I'll try to learn more about that and remove the additional option if possible.~~

(I now understand that this should be addressed by the direct filename imports you mentioned previously; I'll try that out soon)

This reverts commit c64cf92.

…n ESM module override test" This reverts commit 1093df8.

This avoids ERR_UNSUPPORTED_DIR_IMPORT, without having to provide the 'experimental-specifier-resolution' flag to ts-node

cspotcode · 2021-06-25T19:56:27Z

Test are failing on 14.13.0 and 12.15, but I think I know why. I believe we test on these versions specifically because they are old enough that they do not throw an error when attempting to require() ESM; instead they allow it to be executed as CJS. We'll need to figure out what the correct behavior is in those situations. I assume we should either tweak ts-node to throw the error, respecting the moduleTypes overrides, or disable these unit tests on those versions, because users can reasonably be expected to upgrade to the latest node 12 or 14 to leverage newer ts-node features.

jayaddison · 2021-06-26T19:09:58Z

Thanks @cspotcode - I guess that kind of require of ESM-as-CJS-by-default might only work by chance, if at all?

Looking at some of the code comments (particularly this one), would it be fair to say that ts-node should try to do what node itself does?

(in this case currently, that's to throw an ERR_UNKNOWN_FILE_EXTENSION type error exception under Node 12 due to the .ts extension..)

jayaddison · 2021-06-26T19:26:46Z

(in this case currently, that's to throw an ERR_UNKNOWN_FILE_EXTENSION type error exception under Node 12 due to the .ts extension..)

...I've since guessed that that's probably not a sensible error to reproduce, because that was encountered when running the test.esm.js code without any preprocessing / transpilation (if that's the correct term?) of the imported .ts files into JavaScript.

cspotcode · 2021-06-26T21:29:16Z

The goal is to match node's behavior. The behavior matching is as follows:

.ts, .tsx, .js, .jsx files: mimic node's handling of .js files, as if the file had the .js extension
.ts, .tsx, .jsx that match a moduleTypes override:
- if the override is CJS: behave as if the file had the .cjs extension. Whatever node would do for a .cjs file, we do the same
- if the override is ESM: behave as if the file had the .mjs extension. Whatever node would do for a .mjs file, we do the same

We must behave the same way that node would, which means:

if node would throw an error when attempting to require the file, we should throw the same error
if node would execute the file as CJS, we should do the same
if node would execute the file as ESM, we should do the same

In order to correctly match node's behavior with the ERR_REQUIRE_ESM error, which has changed over time, we check node's version number:
https://github.com/TypeStrong/ts-node/blob/main/src/index.ts#L33-L38
(we also version switch in one other place, but that is for a different reason)

cspotcode · 2021-06-26T21:56:06Z

I just realized your comment was about ERR_UNKNOWN_FILE_EXTENSION, not about ERR_REQUIRE_ESM. My mistake. I've edited my last comment to say ERR_REQUIRE_ESM since that is what I was referring to.

The goal regarding ERR_UNKNOWN_FILE_EXTENSION is to teach node what to do for .jsx, .ts, and .tsx extensions so that it doesn't have to throw that error when it encounters those files. I agree with you that it is not a sensible error to reproduce, because we want to teach node to avoid that error.

jayaddison · 2021-06-26T22:34:34Z

No problem at all; we're on the same page, I think - and it's better to overcommunicate in these kinds of conversation.

If I understand correctly then we'll end up with a test matrix of six possible outcomes when moduleTypes overrides are in place:

Two outcomes like the ones that pass already for recent Node versions (1x CJS, 1x ESM)
Two outcomes for Node ~= 14.13 (1x CJS, 1x ESM)
Two outcomes for Node ~= 12.15 (1x CJS, 1x ESM)

It's possible these collapse down to four outcomes, but it'll be a little while before I have time to confirm behaviour across both Node 12.15 and Node 14.13.

I also get the feeling that the test coverage here might end up being a base level of coverage rather than exhaustive; in other words, it's not going to reliably test all the differences that could occur for varying CJS and EJS file content. Still, making sure that each line of code is exercised seems like a worthwhile improvement.

Feel free to continue on further with the branch if you will have more time and/or want to progress this soon. I should have some time outside work hours next week to look at this again.

jayaddison · 2021-07-04T19:09:00Z

I've added 'expected failure' cases for the moduleType override test under Node 12.15, 14.13 - but also have to admit I haven't yet gone upstream to Node to figure out if those are really the ranges affected (i.e. why the failures on those versions as of 7b6daa3 occurred for those unit test runs).

cspotcode · 2021-07-07T14:28:11Z

That's fine, I feel confident those node versions are in the test matrix for this reason. There may be some comments in the GH actions workflow file; I'm not sure. FWIW `n` and `volta` make it fast and easy to switch node versions. I use Volta for all this testing. Is there anything else required here? Tests don't need to be perfect as long as we think they are proving that the feature works.

…

On Sun, Jul 4, 2021, 3:09 PM James Addison ***@***.***> wrote: I've added 'expected failure' cases for the moduleType override test under Node 12.15, 14.13 - but also have to admit I haven't yet gone upstream to Node to figure out if those are really the ranges affected (i.e. why the failures on those versions as of 7b6daa3 <7b6daa3> occurred for those unit test runs). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1376 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC35OB6WPIA7K52LJYJ2L3TWCWVRANCNFSM47ALMQTQ> .

jayaddison · 2021-07-07T18:19:24Z

Is there anything else required here?

I don't think so? Although I do feel a bit out of my depth here.

cspotcode · 2021-07-07T18:35:14Z

@jayaddison ok, thanks, I'll give this a proper review as soon as I have time, and hopefully get this merged.

jayaddison · 2021-07-07T21:32:44Z

Thanks in advance @cspotcode, and take your time; glad to make any further adjustments to make sure the code's acceptable.

* Add moduleType option to override module type on certain files. Also refactor resolverFunctions into their own file; should break this into 2x PRs later. * lint fix * add test * remove unnecessary exports from ts-internals; mark exports as @internal * add docs * strip optionBasePaths from showConfig output * proper path normalization to support windows * lint-fix * use es2015 instead of es2020 in test tsconfig to support ts2.7 * add missing path normalization when calling classifyModule * Test coverage: moduleType overrides during ts-node loader usage (#1376) * Test coverage: add test case to confirm that moduleType overrides are applied for ts-node in loader mode * Ensure that a default export exists for the esm-exception module * Re-order tsconfig.json glob rules, and use implicit globbing * lint fixup: apply prettier * Add 'experimental-specifier-resolution' flag to NPM options in ESM module override test * Ensure that a default export exists for the cjs-subdir module * Revert "Ensure that a default export exists for the cjs-subdir module" This reverts commit c64cf92. * Revert "Add 'experimental-specifier-resolution' flag to NPM options in ESM module override test" This reverts commit 1093df8. * Specify tsconfig project using TS_NODE_PROJECT environment variable * Use full file paths in preference to directory-default module imports This avoids ERR_UNSUPPORTED_DIR_IMPORT, without having to provide the 'experimental-specifier-resolution' flag to ts-node * Update index.ts * Update index.ts * Update tsconfig.json * Extract execModuleTypeOverride function * Add expected failure cases for Node 12.15, 14.13 Co-authored-by: Andrew Bradley <cspotcode@gmail.com> * Update tests * fix * fix * fix for TS2.7 * fix * fix * reword * update docs * address todos * fix Co-authored-by: James Addison <jay@jp-hosting.net>

Test coverage: add test case to confirm that moduleType overrides are…

c9c443c

… applied for ts-node in loader mode

jayaddison added 2 commits June 23, 2021 21:12

Ensure that a default export exists for the esm-exception module

146b019

Re-order tsconfig.json glob rules, and use implicit globbing

42eec0a

jayaddison added 3 commits June 23, 2021 21:20

lint fixup: apply prettier

f8cbc42

Add 'experimental-specifier-resolution' flag to NPM options in ESM mo…

1093df8

…dule override test

Ensure that a default export exists for the cjs-subdir module

c64cf92

jayaddison and others added 7 commits June 24, 2021 00:59

Revert "Ensure that a default export exists for the cjs-subdir module"

1140557

This reverts commit c64cf92.

Revert "Add 'experimental-specifier-resolution' flag to NPM options i…

353fceb

…n ESM module override test" This reverts commit 1093df8.

Specify tsconfig project using TS_NODE_PROJECT environment variable

11f3739

Use full file paths in preference to directory-default module imports

74926a9

This avoids ERR_UNSUPPORTED_DIR_IMPORT, without having to provide the 'experimental-specifier-resolution' flag to ts-node

Update index.ts

f9ed09c

Update index.ts

ca425ef

Update tsconfig.json

7b6daa3

jayaddison added 2 commits July 4, 2021 20:03

Extract execModuleTypeOverride function

22a04de

Add expected failure cases for Node 12.15, 14.13

c0a954f

jayaddison marked this pull request as ready for review July 7, 2021 18:13

cspotcode mentioned this pull request Jul 9, 2021

Add moduleType option to override module type on certain files. #1371

Merged

4 tasks

cspotcode merged commit e0f460e into TypeStrong:ab/override-module-type Jul 9, 2021

jayaddison deleted the ab/override-module-type branch July 9, 2021 07:42

cspotcode added this to the 10.x.x milestone Jul 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test coverage: moduleType overrides during ts-node loader usage #1376

Test coverage: moduleType overrides during ts-node loader usage #1376

jayaddison commented Jun 20, 2021

cspotcode commented Jun 23, 2021

codecov bot commented Jun 23, 2021 •

edited

Loading

jayaddison commented Jun 23, 2021

cspotcode commented Jun 23, 2021 •

edited

Loading

jayaddison commented Jun 23, 2021

jayaddison commented Jun 23, 2021

cspotcode commented Jun 23, 2021

jayaddison commented Jun 23, 2021 •

edited

Loading

cspotcode commented Jun 25, 2021 •

edited

Loading

jayaddison commented Jun 26, 2021

jayaddison commented Jun 26, 2021

cspotcode commented Jun 26, 2021 •

edited

Loading

cspotcode commented Jun 26, 2021

jayaddison commented Jun 26, 2021

jayaddison commented Jul 4, 2021

cspotcode commented Jul 7, 2021 via email

jayaddison commented Jul 7, 2021

cspotcode commented Jul 7, 2021

jayaddison commented Jul 7, 2021

Test coverage: moduleType overrides during ts-node loader usage #1376

Test coverage: moduleType overrides during ts-node loader usage #1376

Conversation

jayaddison commented Jun 20, 2021

cspotcode commented Jun 23, 2021

codecov bot commented Jun 23, 2021 • edited Loading

Codecov Report

jayaddison commented Jun 23, 2021

cspotcode commented Jun 23, 2021 • edited Loading

jayaddison commented Jun 23, 2021

jayaddison commented Jun 23, 2021

cspotcode commented Jun 23, 2021

jayaddison commented Jun 23, 2021 • edited Loading

cspotcode commented Jun 25, 2021 • edited Loading

jayaddison commented Jun 26, 2021

jayaddison commented Jun 26, 2021

cspotcode commented Jun 26, 2021 • edited Loading

cspotcode commented Jun 26, 2021

jayaddison commented Jun 26, 2021

jayaddison commented Jul 4, 2021

cspotcode commented Jul 7, 2021 via email

jayaddison commented Jul 7, 2021

cspotcode commented Jul 7, 2021

jayaddison commented Jul 7, 2021

codecov bot commented Jun 23, 2021 •

edited

Loading

cspotcode commented Jun 23, 2021 •

edited

Loading

jayaddison commented Jun 23, 2021 •

edited

Loading

cspotcode commented Jun 25, 2021 •

edited

Loading

cspotcode commented Jun 26, 2021 •

edited

Loading