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

Allow specifying "require" option via tsconfig #925

Merged
merged 29 commits into from Jul 30, 2020

Conversation

cspotcode
Copy link
Collaborator

@cspotcode cspotcode commented Dec 14, 2019

A follow-up to #921, moved into a separate PR per review comments. The diff for this will look much better after #921 is merged.

Allows specifying --require options via tsconfig.

Questions

  • create() and register() will parse the option from tsconfig. Should they also do the require()s? Right now that's only done by bin

@coveralls
Copy link

@coveralls coveralls commented Dec 14, 2019

Coverage Status

Coverage increased (+1.7%) to 83.636% when pulling 6fb4b8f on cspotcode:ts-node-config-requires into 866bce6 on TypeStrong:master.

@coveralls
Copy link

@coveralls coveralls commented Dec 14, 2019

Coverage Status

Coverage decreased (-21.9%) to 62.105% when pulling e7690b9 on cspotcode:ts-node-config-requires into 2ba0a8f on TypeStrong:master.

@cspotcode
Copy link
Collaborator Author

@cspotcode cspotcode commented Feb 11, 2020

I came across another use-case for this: using ts-node with tsconfig-paths via shebang.

#!/usr/bin/env ts-script
// Shebang loads ts-node options from local tsconfig.json
import * as localFooPackage from 'packages/foo';
// in tsconfig.json
{
  "ts-node": {
    "requires": ["tsconfig-paths/register"]
  },
  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "packages/*": "./local-packages/*" // This example might not be perfect; I haven't used "paths" in a while
    }
  }
}

The alternative is to import tsconfig-paths/register at the top of every entry-point file. Which I'll grant isn't the worst thing in the world, but it's a little bit messy.

@ArkadyDR
Copy link

@ArkadyDR ArkadyDR commented Jul 27, 2020

Hi @cspotcode,

I've found myself wanting this behaviour in one of my projects - for a similar reason to the use case you outlined above. I'd like to use tsconfig-paths with some paths defined in tsconfig.json, but want developers to be able to run some loose scripts (that may import some shared code using paths) via npx ts-node <script path> without needing to bolt on the -r tsconfig-paths/register. As you mentioned I could put the register at the top of each of the scripts, but given the tsconfig.json file specifies the paths it'd be nice if any execution using that project could load the requisite module for understanding the project (and perhaps any other bootstrapper code common to the project).

Given the age, are you likely to finish this PR? If not I might attempt something similar when I get some free time!

@cspotcode
Copy link
Collaborator Author

@cspotcode cspotcode commented Jul 27, 2020

@ArkadyDR I've merged the latest master so this PR is ready for another review.

blakeembrey and I didn't agree whether or not this should be a feature, so I shelved it in order to get #921 released. I still think it's a good feature for the reasons that you cite, but we'll need to come to agreement.

@blakeembrey, want to revisit this?

@cspotcode cspotcode changed the title Allow specifying "requires" option via tsconfig Allow specifying "require" option via tsconfig Jul 28, 2020
@codecov
Copy link

@codecov codecov bot commented Jul 28, 2020

Codecov Report

Merging #925 into master will increase coverage by 0.28%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #925      +/-   ##
==========================================
+ Coverage   74.91%   75.20%   +0.28%     
==========================================
  Files           6        6              
  Lines         606      617      +11     
  Branches      141      146       +5     
==========================================
+ Hits          454      464      +10     
  Misses        100      100              
- Partials       52       53       +1     
Flag Coverage Δ
#node_10 72.55% <78.57%> (+0.19%) ⬆️
#node_12_15 72.93% <78.57%> (+0.19%) ⬆️
#node_12_16 72.93% <78.57%> (+0.19%) ⬆️
#node_13 74.87% <78.57%> (+0.12%) ⬆️
#node_14 74.87% <78.57%> (+0.12%) ⬆️
#typescript_2_7 74.71% <78.57%> (+0.29%) ⬆️
#typescript_latest 73.90% <78.57%> (+0.30%) ⬆️
#typescript_next 73.74% <78.57%> (+0.31%) ⬆️
#ubuntu 74.87% <78.57%> (+0.29%) ⬆️
#windows 75.04% <78.57%> (+0.28%) ⬆️

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

Impacted Files Coverage Δ
src/index.ts 77.45% <76.92%> (+0.21%) ⬆️
src/bin.ts 70.34% <100.00%> (-0.18%) ⬇️
src/esm.ts 68.88% <0.00%> (+2.22%) ⬆️

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 866bce6...6fb4b8f. Read the comment docs.

@cspotcode
Copy link
Collaborator Author

@cspotcode cspotcode commented Jul 28, 2020

CI seems to be stuck but as far as I can tell all tests are fixed. It properly resolves require() targets relative to the tsconfig file. And I renamed the option from requires to require to match the CLI flag.

@blakeembrey
Copy link
Member

@blakeembrey blakeembrey commented Jul 29, 2020

want to revisit this?

Absolutely, sounds like a good idea now that we shipped the basic feature.

src/bin.ts Outdated
@@ -199,6 +199,8 @@ export function main (argv: string[]) {
: undefined
})

const requires = argsRequire.length !== 0 ? argsRequire : service.options.require || []
Copy link
Member

@blakeembrey blakeembrey Jul 29, 2020

Choose a reason for hiding this comment

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

Should we merge these lists together? Seems odd to have the CLI entirely overwrite the options when specified.

Copy link
Collaborator Author

@cspotcode cspotcode Jul 29, 2020

Choose a reason for hiding this comment

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

Yeah, that makes sense. I can't think of a good use-case where you'd want the --require flag to suppress whatever was specified in the tsconfig.

@@ -991,6 +1005,14 @@ function readConfig (
useCaseSensitiveFileNames: ts.sys.useCaseSensitiveFileNames
}, basePath, undefined, configFileName))

if (tsconfigOptions.require) {
Copy link
Member

@blakeembrey blakeembrey Jul 29, 2020

Choose a reason for hiding this comment

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

Thoughts on doing the require for these options within index.ts so configuration flows in a single direction? It might make it easier instead of needing to resolve and pass it back to the CLI which requires. Also, this would ensure we cover the programmatic usage of register() and via ts-node/register.

Copy link
Collaborator Author

@cspotcode cspotcode Jul 29, 2020

Choose a reason for hiding this comment

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

Yeah I like that.

The CLI uses Module._preloadModules; do you know if that function is doing anything special that will break if we switch to a normal require() call? There's nothing wrong with _preloadModules but I don't really understand what it does other than require() stuff.

Copy link
Member

@blakeembrey blakeembrey Jul 29, 2020

Choose a reason for hiding this comment

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

Yeah, I recall I tried to change it one time. Feel free to keep the CLI logic as it is and build the internal one separately for now so there's no regression. Here's the implementation in node.js: https://github.com/nodejs/node/blob/ec2ffd6b9d255e19818b6949d2f7dc7ac70faee9/lib/internal/modules/cjs/loader.js#L1400-L1417.

Here's where I had changed it at one point, but I can't seem to find the issue. I should have linked things better in the past 🤕 2e99c50#diff-ee45801c137c3b9f9d8ec7ece340a514R197.

Copy link
Member

@blakeembrey blakeembrey Jul 29, 2020

Choose a reason for hiding this comment

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

The main thing it's trying to ensure is that the require() is occurring from the place where you run the script, not where ts-node lives.

Copy link
Collaborator Author

@cspotcode cspotcode Jul 29, 2020

Choose a reason for hiding this comment

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

Ok, I don't think there's anything wrong with calling _preloadModules from within register(), so I'll keep doing that.

Should it happen in register() or in create()? I feel like register() makes more sense because that's where we "install" the environment. But source map support is installed in create().

@ArkadyDR
Copy link

@ArkadyDR ArkadyDR commented Jul 29, 2020

@ArkadyDR I've merged the latest master so this PR is ready for another review.

Thanks @cspotcode and @blakeembrey for jumping on this so quickly!

Yeah, that makes sense. I can't think of a good use-case where you'd want the --require flag to suppress whatever was specified in the tsconfig.

If it helps, my expectation would be in line with where you're going - I would think that any CLI requires would be in addition to what is in the config, although you may want to de-duplicate them if there would be an overhead in processing something twice? If someone had a case where one of the configured requires was innapropriate for a script, the solution would probably be to spin off one or more extra tsconfig files (using the extension behaviour).

@cspotcode
Copy link
Collaborator Author

@cspotcode cspotcode commented Jul 29, 2020

Ok, here's how the new behavior works:

create() will accept a require array in the programmatic options, merge it with the array it pulls from tsconfig.json, but will not require() anything.

register() will _preloadModules() the array it gets from create().

When create() merges the arrays, tsconfig items are first, then the programmatic items / those from the CLI --require. TBH I couldn't think of a good reason to prefer one ordering over the other.

Items pulled from tsconfig.json will be eagerly resolved at parse-time to absolute paths via createRequire(pathToTheTsConfigFile).resolve(item). It will be as if the tsconfig file had required them.

Programmatically passed items are not resolved and are passed verbatim to _preloadModules. This matches the current CLI behavior for --require. Docs point out that you can pre-resolve and pass us absolute paths for best results.

@cspotcode cspotcode requested a review from blakeembrey Jul 30, 2020
@cspotcode cspotcode merged commit 436e3a8 into TypeStrong:master Jul 30, 2020
23 checks passed
@cspotcode cspotcode moved this from To Do to Merged to master in Andrew Bradley's Tasks Aug 13, 2020
@cspotcode cspotcode mentioned this pull request Aug 21, 2020
4 tasks
@cspotcode
Copy link
Collaborator Author

@cspotcode cspotcode commented Aug 21, 2020

@ArkadyDR this is released in v9.0.0: https://github.com/TypeStrong/ts-node/releases/tag/v9.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Andrew Bradley's Tasks
Merged to master
Development

Successfully merging this pull request may close these issues.

None yet

4 participants