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

Respect tsconfig.json extends when validating config #5537

Merged
merged 3 commits into from
Oct 23, 2018

Conversation

ianschmitz
Copy link
Contributor

This PR enhances #5532 by adding support for extends in tsconfig.json. We use the TypeScript compiler to parse the configuration, which traverses the relationships.

Some interesting tidbits:

The output from ts.readConfigFile() is mutated after running ts.parseJsonConfigFileContent() which merges in include and exclude from extended config files, but leaves the compilerOptions untouched:
image

We then use ts.parseJsonConfigFileContent() which gives us a parsed options among other things. For example you can see how module is missing from raw, but is present in options, as it was included by extending tsconfig.test.json:

{
    options: {
        pretty: true,
        target: 1,
        allowJs: true,
        skipLibCheck: true,
        esModuleInterop: true,
        allowSyntheticDefaultImports: true,
        strict: true,
        module: 6,
        moduleResolution: 2,
        resolveJsonModule: true,
        isolatedModules: true,
        noEmit: true,
        jsx: 1,
        configFilePath: undefined,
    },
    fileNames: [
        "C:/Projects/create-react-app/packages/react-scripts/template/src/App.js",
        "C:/Projects/create-react-app/packages/react-scripts/template/src/index.tsx",
        "C:/Projects/create-react-app/packages/react-scripts/template/src/react-app.d.ts",
        "C:/Projects/create-react-app/packages/react-scripts/template/src/serviceWorker.d.ts",
        "C:/Projects/create-react-app/packages/react-scripts/template/src/serviceWorker.js",
    ],
    projectReferences: undefined,
    typeAcquisition: { enable: false, include: [], exclude: [] },
    raw: {
        extends: "./tsconfig.test.json",
        compileOnSave: false,
        compilerOptions: {
            target: "es5",
            allowJs: true,
            skipLibCheck: true,
            esModuleInterop: true,
            allowSyntheticDefaultImports: true,
            strict: true,
            moduleResolution: "node",
            resolveJsonModule: true,
            isolatedModules: true,
            noEmit: true,
            jsx: "preserve",
        },
    },
    errors: [],
    wildcardDirectories: {
        "C:/Projects/create-react-app/packages/react-scripts/template/src": 1,
    },
    compileOnSave: false,
    configFileSpecs: {
        filesSpecs: undefined,
        includeSpecs: ["src"],
        excludeSpecs: ["**/__tests__/**", "**/?*test.*", "**/?*spec.*"],
        validatedIncludeSpecs: ["src"],
        validatedExcludeSpecs: [
            "**/__tests__/**",
            "**/?*test.*",
            "**/?*spec.*",
        ],
        wildcardDirectories: {
            "C:/Projects/create-react-app/packages/react-scripts/template/src": 1,
        },
    },
}

@Timer Timer added this to the 2.1 milestone Oct 23, 2018
@@ -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.

// 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?

@ianschmitz ianschmitz mentioned this pull request Oct 23, 2018
3 tasks
@eps1lon
Copy link
Contributor

eps1lon commented Oct 23, 2018

Really excited about typescript support in vanilla create-react-app. Great work 👍

It would be nice to include a helpful error message when parsing failed.

You could add

const formatDiagnosticHost = {
  getCanonicalFileName: fileName => fileName,
  getCurrentDirectory: ts.sys.getCurrentDirectory,
  getNewLine: () => os.EOL,
};
- throw error
+ throw new Error(ts.formatDiagnostic(error, formatDiagnosticHost));
- throw result.errors[0]
+ ts.formatDiagnostics(result.errors, formatDiagnosticHost)

and then add that error to console.error. This way you get something like
Error: tsconfig.json(15,21): error TS1002: Unterminated string literal.

Without ts.formatDiagnostic it would only say TypeError: error is not a constructor

If this is out of scope for this PR I can open a separate PR. I already added it locally.

@ianschmitz
Copy link
Contributor Author

Really excited about typescript support in vanilla create-react-app. Great work 👍

It would be nice to include a helpful error message when parsing failed.

You could add

const formatDiagnosticHost = {
  getCanonicalFileName: fileName => fileName,
  getCurrentDirectory: ts.sys.getCurrentDirectory,
  getNewLine: () => os.EOL,
};
- throw error
+ throw new Error(ts.formatDiagnostic(error, formatDiagnosticHost));
- throw result.errors[0]
+ ts.formatDiagnostics(result.errors, formatDiagnosticHost)

and then add that error to console.error. This way you get something like
Error: tsconfig.json(15,21): error TS1002: Unterminated string literal.

Without ts.formatDiagnostic it would only say TypeError: error is not a constructor

If this is out of scope for this PR I can open a separate PR. I already added it locally.

@Timer what do you think about this? Currently we're just throwing the error and catching it without displaying any of the error information to the user. I'm happy to help improve this experience if we think the user should see this info.

@Timer
Copy link
Contributor

Timer commented Oct 23, 2018

I'd like to improve the error experience if the above change helps.

@Timer
Copy link
Contributor

Timer commented Oct 23, 2018

@ianschmitz I pushed a work around, what do you think?

@Timer
Copy link
Contributor

Timer commented Oct 23, 2018

See 1feaa5c

@ianschmitz
Copy link
Contributor Author

@ianschmitz I pushed a work around, what do you think?

Ooo i like that. Thanks for doing that!

@eps1lon would you like to create a PR after this one goes in? I don't want to steal the credit 😛

@Timer
Copy link
Contributor

Timer commented Oct 23, 2018

I had added the change for the friendly errors since it was so small, thanks @eps1lon!

@Timer Timer merged commit 315ff4b into facebook:master Oct 23, 2018
@eps1lon
Copy link
Contributor

eps1lon commented Oct 23, 2018

I think everything was covered by 38bd634. If PR credit was so important I would've waited till this get's merged and open then. The sooner TS supports gets release the better.

@ianschmitz ianschmitz deleted the parse-tsconfig branch October 23, 2018 19:57
chanand pushed a commit to chanand/create-react-app that referenced this pull request Oct 25, 2018
* Use TS to resolve tsconfig extends

* Prevent modifications to original tsconfig

* Print friendly error
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Oct 29, 2018
* Use TS to resolve tsconfig extends

* Prevent modifications to original tsconfig

* Print friendly error
nate770 pushed a commit to ONTW/create-react-app that referenced this pull request Oct 30, 2018
* Use TS to resolve tsconfig extends

* Prevent modifications to original tsconfig

* Print friendly error
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants