-
-
Notifications
You must be signed in to change notification settings - Fork 531
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
Configure ts-node via tsconfig #921
Conversation
1 similar comment
693339b
to
7e301ef
Compare
@blakeembrey @G-Rath This is ready for a review. It still needs docs updates but I'd rather write them after the code is reviewed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core functionality is good, but it's relatively straight forward compared to all the other changes. Let's scope down the PR to just the core changes necessary.
package.json
Outdated
"clean": "rimraf dist && rimraf tsconfig.schema.json && rimraf tsconfig.schemastore-schema.json", | ||
"build": "npm run clean && npm run build:tsc && npm run build:configSchema", | ||
"build:tsc": "tsc", | ||
"build:configSchema": "typescript-json-schema --topRef --refs --validationKeywords allOf --out tsconfig.schema.json tsconfig.json TsConfigSchema && node --require ./register ./scripts/create-merged-schema", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Consistent dash casing is currently used.
@@ -0,0 +1,58 @@ | |||
#!/usr/bin/env ts-node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this line doesn't work, since we use node --require
in the script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work if you have ts-node
installed globally, but that's not technically true all the time, so I put node --require
in the package script.
scripts/create-merged-schema.ts
Outdated
*/ | ||
|
||
import axios from 'axios'; | ||
import * as Path from 'path'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Casing on Path
should be path
, but you could just import resolve
only.
}, | ||
}, | ||
allOf: [ | ||
...schemastoreSchema.allOf.slice(0, 4), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean? Why 4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so it gets spliced into the array at a point that looks nice. https://github.com/SchemaStore/schemastore/blob/master/src/schemas/json/tsconfig.json#L630-L631
* by merging the schemastore schema with our ts-node additions. | ||
* This merged schema can be submitted in a pull request to | ||
* SchemaStore. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this script necessary? Why can't we specify a URL within our schema to have it already be up-to-date with the remote schema? I'm worried this will mean that every TypeScript version I'll need to be generating and releasing the updated schema. You're meant to be able to use $ref
for this use-case: https://cswr.github.io/JsonSchema/spec/definitions_references/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, I'm generating 2 schemas. First is tsconfig.schema.json
, which does use $ref
. That was my initial approach, same thinking as you.
@G-Rath suggested patching ts-node's additions into the official file on Schemastore. This script produces that merged schema. The idea is that it would live in SchemaStore, so editors would tab-complete the ts-node fields without any configuration on the user's part. I'm not sure if they'll ever merge it, though, because if someone tab-completes the ts-node options, they'll still need to install ts-node. It could be nice publicity for the library; I dunno.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They will merge it, since it's not a breaking change (they don't reject PRs - like, ever) :)
The configuration options are harmless if ts-node
isn't there.
src/index.ts
Outdated
} | ||
|
||
// compute enough options to read the config file | ||
let { cwd, isScoped, compiler, ts, fileExists, readFile } = recomputedOptions() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be doing recomputedOptions
like this, it feels wasteful. There's only two options here (isScoped
and compiler
) that can change from tsconfig.json
. isScoped
could be populated after tsconfig.json
is loaded, therefore this is really only here to load compiler
again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move cwd
and isScoped
outside of this function.
The other 4 -- compiler
, ts
, readFile
, and fileExists
-- are all needed both before and after readConfig()
. Should I rename the function to something like loadCompiler
? I can also get rid of the destructuring if you want; just assign straight to local let
variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, for readFile
and fileExists
, we should probably just fallback on ts.sys
within the readConfig
function directly - I don't think those two functions are actually used consistently when I looked at the code, so it wouldn't be a loss to lose the functionality altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean. ts.parseJsonConfigFileContent
accepts a ParseConfigHost
, which has readFile
, fileExists
, and readDirectory
. We can add a readDirectory
option to RegisterOptions
, and then we can consistently use readFile
and fileExists
in a way that makes sense.
Who is using these options, and what do they use them for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be cleaner if ts-node accepted a *Host
-style object instead of readFile
and fileExists
. That way consumers of the API could override any of the various fs-related methods on ts.sys
. Looks like there are a bunch, like directoryExists
, that are being used by ts-node but not overridable via the API.
src/index.ts
Outdated
} | ||
|
||
// Merge default options, tsconfig options, and explicit --flag options | ||
options = optionsHelper.merge() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use defaults
directly, there's a layer of indirection and mutation here that's not necessary from what I can tell.
tsconfig.json
Outdated
@@ -1,4 +1,6 @@ | |||
{ | |||
"$schema": "./tsconfig.schemastore-schema.json", | |||
"ts-node": {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it empty for a reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason, I forgot I added this. I'll remove it.
src/index.ts
Outdated
let { cwd, isScoped, compiler, ts, fileExists, readFile } = recomputedOptions() | ||
|
||
// Read config file | ||
const { config, options: tsconfigOptions } = readConfig(cwd, ts, fileExists, readFile, options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could be explicit about options
being passed to readConfig
here. One feature I think is broken due to the current design is the files
option, that functionality could be moved into create
instead of readConfig
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I think we need to set empty arrays for files
and includes
within readConfig
because otherwise TypeScript's parseJsonConfigFileContent
will do a bunch of globbing filesystem calls to discover the full list of files. Does that sound right?
src/index.ts
Outdated
for (const source of sources) { | ||
for (const key of Object.keys(source)) { | ||
const value = (source as any)[key] | ||
if (value !== undefined) merged[key] = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want to do != null
since some options can be null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want a null
to be able to override a non-null value specified elsewhere? The idea was undefined
means the option is unspecified, whereas null
means we explicitly want to override the default with null
. I was mimicking the JS language behavior of defaults in arguments and destructuring. Will that cause problems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will that cause problems?
No, I think your reasoning sounds solid. I hadn't considered someone would use null
, it's historically just an input because of using the yn
library which returns null
when a value is neither true or false.
Thanks for the speedy review. I started working on the necessary changes before realizing I won't be able to get to this till at least next week. I pushed what I have, and I'll do the rest as soon as I can. |
@cspotcode I can also take a stab at landing the PR this weekend too. Since you've done the core functionality, I'd really be fixing all my own nit-picking since I'm particular about the structure 😄 |
@blakeembrey That is totally fine with me, if you've got the free time. Thanks! |
@cspotcode Nope, it's over. I intended to review this now. |
src/index.ts
Outdated
|
||
// Read config file | ||
const { config, options: tsconfigOptions } = readConfig(cwd, ts, optionsWithoutDefaults) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the usage of optionsWithoutDefaults
here is what broke #938. Fixing and adding a test now.
Edit: Actually, this shouldn't be true because it does merge with DEFAULTS
in readConfig
.
Closes #4. Still WIP, but I still welcome early code review.