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.

Sign up for GitHub

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

Jump to bottom

Adding "extends": support to 'ts-node' loading from tsconfig.json #1292

Closed
wants to merge 2 commits into from
Closed

Adding "extends": support to 'ts-node' loading from tsconfig.json #1292

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e771ff
Adding "extends": support to `'ts-node'` loading from `tsconfig.json`…
seivan Apr 7, 2021
3340af3
Resolve extended tsconfig path relative to current config path, not cwd
seivan Apr 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 36 additions & 6 deletions src/configuration.ts
View file
Open in desktop
Expand Up @@ -41,6 +41,36 @@ function fixConfig(ts: TSCommon, config: _ts.ParsedCommandLine) {
return config;
}

function readTsConfigExtendForTsNodeRecursively(
cwd: string,
ts: TSCommon,
rawConfig: { [key: string]: any } = {}
seivan marked this conversation as resolved.
Show resolved Hide resolved
): TsConfigOptions {
const configKey = 'ts-node';
const recurse = (
extending: { [key: string]: any } = {},
original: { [key: string]: any } = {}
) => {
return readTsConfigExtendForTsNodeRecursively(cwd, ts, {
[configKey]: { ...extending[configKey], ...original[configKey] },
});
};

const { extends: extendFile, ...config } = rawConfig;

const result =
extendFile === undefined
? filterRecognizedTsConfigTsNodeOptions(config[configKey])
: [extendFile]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: get rid of the filter().map() chaining. We prefer code like const, if(), for(const a of b), etc.

Copy link
Author

Choose a reason for hiding this comment

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

@cspotcode It's not for everyone 🐼. Once you have a look over the logic itself, I'll swap it out.

.filter((x) => x && typeof x === 'string')
.map((x) => x as string)
.map((extended) => resolve(cwd, extended))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work when ./bar/tsconfig.json refers to ./bar/tsconfig2.json by relative path? Because the path is not relative to cwd? Does it work for "extends": "@tsconfig/node14/tsconfig.json"? (https://github.com/tsconfig/bases)

Copy link
Author

Choose a reason for hiding this comment

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

Regarding relative paths...
Not sure if I want to say yes, I do use this myself and I my cwd wasn't relative, but then again I am doing other stuff that might impact it.
Good call, would do best to extract dirname/basename from current config in order to make relative paths work - but I gotta check first.
There is another approach and that is to let ts itself try to find the file, there is a function that for that last I checked.

Regarding @tsconfig/node14/tsconfig.json
Yeah no chance that would work.
I am not familiar with that, is that something you distribute or TS?
If it's the latter, then I'd reckon there wouldn't be a ts-node key there?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll need tests for this, and we'll need to support typescript's "extends" resolution behavior. @tsconfig/node14 was just an example; our users can choose to "extends" from anywhere that typescript supports, and we'll need to match that behavior. Ideally, we figure out how TypeScript is doing this logic and do the exact same, using as many TypeScript APIs as possible to avoid duplication.

https://www.typescriptlang.org/tsconfig#extends

Copy link
Author

@seivan seivan Apr 9, 2021 •
edited

Choose a reason for hiding this comment

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

Right, if TS exposes that, then I guess best bet is to use it, but to reimplement it is overkill.
In general, I think the simplest approach is just following the links.

In regards to your previous points about relative paths, this is an easy fix:

.map(path => isAbsolute(path) ? [path] : [currentConfigPath, path])
.map(paths => resolve(paths)

However I don't care much for extends: "@tsconfig/node14/tsconfig.json", on the premise that I just don't approve of this path they're going down on.

Copy link
Author

Choose a reason for hiding this comment

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

Using relative paths and not CWD now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

However I don't care much for extends: "@tsconfig/node14/tsconfig.json", on the premise that I just don't approve of this path they're going down on.

Unfortunately tsc already does this so we need to match. Otherwise it gets confusing and we need to document how our behavior deviates, and I'll be the one answering the issues that get files.

I certainly understand why you might choose not to use this feature on your projects, but for ts-node, we need to match TSC's behavior or else we're making things confusing.

.map((pathString) => ts.readConfigFile(pathString, ts.sys.readFile))
.map((readFile) => readFile.config)
.map((extendedConfig) => recurse(extendedConfig, config))[0];

return result ?? {};
}
/**
* Load TypeScript configuration. Returns the parsed TypeScript config and
* any `ts-node` options specified in the config file.
Expand Down Expand Up @@ -105,11 +135,11 @@ export function readConfig(
}

// Fix ts-node options that come from tsconfig.json
const tsNodeOptionsFromTsconfig: TsConfigOptions = Object.assign(
{},
filterRecognizedTsConfigTsNodeOptions(config['ts-node'])
const tsNodeOptionsFromTsconfig: TsConfigOptions = readTsConfigExtendForTsNodeRecursively(
cwd,
ts,
config
);

// Remove resolution of "files".
const files =
rawApiOptions.files ?? tsNodeOptionsFromTsconfig.files ?? DEFAULTS.files;
Expand Down Expand Up @@ -179,8 +209,8 @@ export function readConfig(
*/
function filterRecognizedTsConfigTsNodeOptions(
jsonObject: any
): TsConfigOptions {
if (jsonObject == null) return jsonObject;
): TsConfigOptions | undefined {
if (jsonObject == null) return undefined;
const {
compiler,
compilerHost,
Expand Down