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

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

Merged
merged 22 commits into from Jul 9, 2021

Conversation

cspotcode
Copy link
Collaborator

@cspotcode cspotcode commented Jun 10, 2021

Attempting an implementation of #1342
Closes #1342

Be sure @jayaddison gets credit for #1376 in the release notes.

Also refactors createResolverFunctions into its own file; should break this into 2x PRs later.

TODOs

  • mark all exported ts-internals.ts as @internal
  • check docs; add missing
  • Make --showConfig moduleTypes log correct, relative paths same as include (not done, but extracted to Make --showConfig moduleTypes log correct, relative paths same as include #1389)
    • When include is specified in an extended tsconfig in another directory, --showConfig correctly updates relative paths so they render correctly.
  • address missing coverage with test cases

refactor resolverFunctions into their own file; should break this into
2x PRs later.
@cspotcode cspotcode marked this pull request as draft June 10, 2021 06:24
@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #1371 (1ec6b05) into main (4e7fcb7) will decrease coverage by 2.74%.
The diff coverage is 68.03%.

Impacted Files Coverage Δ
src/bin.ts 91.40% <ø> (ø)
src/ts-internals.ts 47.94% <46.82%> (-10.68%) ⬇️
src/resolver-functions.ts 84.09% <84.09%> (ø)
src/index.ts 78.70% <92.30%> (+0.27%) ⬆️
src/module-type-classifier.ts 92.30% <92.30%> (ø)
dist-raw/node-cjs-loader-utils.js 90.38% <100.00%> (+0.58%) ⬆️
src/configuration.ts 93.82% <100.00%> (+0.23%) ⬆️
src/esm.ts 87.50% <100.00%> (+2.39%) ⬆️

@jayaddison
Copy link
Contributor

👋 thanks for the help in #1337 @cspotcode - glad to offer a bit of help in return (or to try to, at least :)). If you could provide some of that guidance re: test cases I'll start to take a look soon.

@cspotcode
Copy link
Collaborator Author

Since the coverage report is pretty big, I'll highlight some specific lines and the tests that will cover them. Please don't let the verbosity scare you; I erred on the side of including too much detail.

Missing coverage: https://app.codecov.io/gh/TypeStrong/ts-node/compare/1371/diff#D2L115

The test I wrote for this PR (https://github.com/TypeStrong/ts-node/pull/1371/files#diff-1d9efcccb250238d73480e5c49afae61494dd3103e9a16c38d36979f27c6ff90R1468-R1474) tests when you try to require() a script from within a CommonJS file. The overrides are checked to see if the target should follow the package.json or if it should be overridden to be CJS or ESM.

However, there's another code path I neglected to test: when you try to import something (not require) from within an ESM file using the --loader. In this situation, the overrides still need to be checked the same way as above. The missing coverage lines linked above indicate we are not testing this codepath. We need to be sure that overrides can force a file to be treated as CJS or as ESM. Either should load, but the CJS script will have typeof require === 'function' and the ESM script will have typeof require === 'undefined'.

In the linked code coverage, those 2x return statements are when there's an override forcing a file to be CJS and ESM, respectively.

I believe you should be able to copy-paste the test I wrote and then modify it to test this new situation. You can see I added a ./tests/module-types directory which is a mini-project, and then the tests are running ts-node in that directory and verifying that the output looks correct. In the mini-project, I've written scripts that will intentionally fail if the CJS/ESM overrides are not working correctly.

When our tests run, they actually npm install ts-node into the tests subdirectory, which makes them a close simulation of real usage. It also means you can play around by doing cd ./tests/module-types and then running commands such as ../node_modules/.bin/ts-node ... or node --loader ts-node/esm .... That should help to quickly create the test cases because you won't need to run ava to confirm it's working.

@jayaddison
Copy link
Contributor

Getting closer to having the additional test coverage available to offer here.

One question so far: when using node --loader ts-node/esm ... as the command-under-test, is there a way to indicate the path to the tsconfig.json file, similar to the --project flag when using the ts-node command directly?

@cspotcode
Copy link
Collaborator Author

cspotcode commented Jun 20, 2021 via email

jayaddison and others added 7 commits July 9, 2021 00:30
* 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>
@jayaddison
Copy link
Contributor

Thanks for the credit offer @cspotcode; only required if the tests end up being enabled in the test suite. I'll take a look into the volta Node version manager soon, but don't wait on me (and feel free to remove the commented tests (and credit) if they're not ready; less noise in the codebase).

@cspotcode
Copy link
Collaborator Author

cspotcode commented Jul 9, 2021 via email

@cspotcode
Copy link
Collaborator Author

Only failure is a drop in code coverage, but I've reviewed the coverage report to determine that everything important is covered.

@cspotcode cspotcode merged commit 1bc470d into main Jul 9, 2021
@cspotcode cspotcode added this to the 10.x.x milestone Jul 9, 2021
@cspotcode cspotcode deleted the ab/override-module-type branch October 11, 2021 03:47
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.

Flag for ESM loader to override package.json "type"
2 participants