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

Respect tsconfig.json extends when validating config #5537

Merged
merged 3 commits into from
Oct 23, 2018
Merged

Respect tsconfig.json extends when validating config #5537

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6a1235
Use TS to resolve tsconfig extends
ianschmitz Oct 23, 2018
1feaa5c
Prevent modifications to original tsconfig
Timer Oct 23, 2018
38bd634
Print friendly error
Timer Oct 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 58 additions & 22 deletions packages/react-scripts/scripts/utils/verifyTypeScriptSetup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,25 +19,6 @@ function writeJson(fileName, object) {
fs.writeFileSync(fileName, JSON.stringify(object, null, 2) + os.EOL);
}

const compilerOptions = {
// These are suggested values and will be set when not present in the
// tsconfig.json
target: { suggested: 'es5' },
allowJs: { suggested: true },
skipLibCheck: { suggested: true },
esModuleInterop: { suggested: true },
allowSyntheticDefaultImports: { suggested: true },
strict: { suggested: true },

// These values are required and cannot be changed by the user
module: { value: 'esnext', reason: 'for import() and import/export' },
moduleResolution: { value: 'node', reason: 'to match webpack resolution' },
resolveJsonModule: { value: true, reason: 'to match webpack loader' },
isolatedModules: { value: true, reason: 'implementation limitation' },
noEmit: { value: true },
jsx: { value: 'preserve', reason: 'JSX is compiled by Babel' },
};

function verifyTypeScriptSetup() {
let firstTimeSetup = false;

Expand Down Expand Up @@ -86,8 +67,44 @@ function verifyTypeScriptSetup() {
process.exit(1);
}

const compilerOptions = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this object moved? Seems like unnecessary diff noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done to add in the parsedValue properties, which is using the typescript compiler reference. Alternatively we could leave it in place and hard code the values from typescript. Here's an example:

    enum ModuleKind {
        None = 0,
        CommonJS = 1,
        AMD = 2,
        UMD = 3,
        System = 4,
        ES2015 = 5,
        ESNext = 6
    }

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not hard code. I'm fine with this move.

// These are suggested values and will be set when not present in the
// tsconfig.json
// 'parsedValue' matches the output value from ts.parseJsonConfigFileContent()
target: {
parsedValue: ts.ScriptTarget.ES5,
suggested: 'es5',
},
allowJs: { suggested: true },
skipLibCheck: { suggested: true },
esModuleInterop: { suggested: true },
allowSyntheticDefaultImports: { suggested: true },
strict: { suggested: true },

// These values are required and cannot be changed by the user
module: {
parsedValue: ts.ModuleKind.ESNext,
value: 'esnext',
reason: 'for import() and import/export',
},
moduleResolution: {
parsedValue: ts.ModuleResolutionKind.NodeJs,
value: 'node',
reason: 'to match webpack resolution',
},
resolveJsonModule: { value: true, reason: 'to match webpack loader' },
isolatedModules: { value: true, reason: 'implementation limitation' },
noEmit: { value: true },
jsx: {
parsedValue: ts.JsxEmit.Preserve,
value: 'preserve',
reason: 'JSX is compiled by Babel',
},
};

const messages = [];
let tsconfig;
let parsedOptions;
try {
const { config, error } = ts.readConfigFile(
paths.appTsConfig,
Expand All @@ -99,6 +116,21 @@ function verifyTypeScriptSetup() {
}

tsconfig = config;

// Get TS to parse and resolve any "extends"
// Calling this function also mutates the tsconfig above,
// adding in "include" and "exclude", but the compilerOptions remain untouched
const result = ts.parseJsonConfigFileContent(
config,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be tsconfig for clarity. Kinda sucks that mutation happens, but I'm OK with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, we can't mutate this here. If we write out the tsconfig the file will include the include and exclude options which would be unexpected. Can we deep clone tsconfig before passing it to this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes good point about it getting written out! Definitely need to fix that.

Yep we can deep clone it. We will likely need to create a couple extra variables then to hold on to the include and exclude that is mutated on the clone, as below we're relying on tsconfig.include and tsconfig.exclude to check.

Sound good?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I follow. We should be fine to clone the entire thing without special behavior.

All checks are going to be made against this new result object AFAIK. Only changes (no reads) will be made against the original tsconfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yeah let me think through this a bit...

So currently tsconfig is what was read from the tsconfig.json. When we call ts.parseJsonConfigFileContent() it mutates the provided tsconfig, and adds in the include and exclude properties that came in from extended tsconfig config files (I find this to be a strange behavior. Their API for reading the config files isn't the most intuitive). tsconfig.compilerOptions is still the original raw properties from the root tsconfig.json.

parsedOptions contains the compilerOptions that was parsed from reading in all the extended tsconfig.json files.

If we wanted to clone tsconfig before passing it into ts.parseJsonConfigFileContent(), it means we wouldn't know the include and exclude values below when doing the if (tsconfig.include == null) { ... } check, as the include and exclude could be part of one of the extended files as you can see in my screenshot in the top post.

I hope that makes sense. What do you think would be the best approach here?

ts.sys,
path.dirname(paths.appTsConfig)
);

if (result.errors && result.errors.length) {
throw result.errors[0];
}

parsedOptions = result.options;
} catch (_) {
console.error(
chalk.red.bold(
Expand All @@ -116,17 +148,20 @@ function verifyTypeScriptSetup() {
}

for (const option of Object.keys(compilerOptions)) {
const { value, suggested, reason } = compilerOptions[option];
const { parsedValue, value, suggested, reason } = compilerOptions[option];

const valueToCheck = parsedValue === undefined ? value : parsedValue;

if (suggested != null) {
if (tsconfig.compilerOptions[option] === undefined) {
if (parsedOptions[option] === undefined) {
tsconfig.compilerOptions[option] = suggested;
messages.push(
`${chalk.cyan('compilerOptions.' + option)} to be ${chalk.bold(
'suggested'
)} value: ${chalk.cyan.bold(suggested)} (this can be changed)`
);
}
} else if (tsconfig.compilerOptions[option] !== value) {
} else if (parsedOptions[option] !== valueToCheck) {
tsconfig.compilerOptions[option] = value;
messages.push(
`${chalk.cyan('compilerOptions.' + option)} ${chalk.bold(
Expand All @@ -137,6 +172,7 @@ function verifyTypeScriptSetup() {
}
}

// tsconfig will have the merged "include" and "exclude" by this point
if (tsconfig.include == null) {
tsconfig.include = ['src'];
messages.push(
Expand Down