Support comments in tsconfig.json

[jackjocross]

Since tsconfig files allow comments, using require for a tsonfig.json with comments results in the following syntax error:

$ react-scripts start
internal/modules/cjs/loader.js:724
    throw err;
    ^

SyntaxError: /Users/jack/dev/github/test/tsconfig.json: Unexpected token / in JSON at position 157
    at JSON.parse (<anonymous>)
    at Object.Module._extensions..json (internal/modules/cjs/loader.js:721:27)
    at Module.load (internal/modules/cjs/loader.js:612:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
    at Function.Module._load (internal/modules/cjs/loader.js:543:3)
    at Module.require (internal/modules/cjs/loader.js:650:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at getModules (/Users/jack/dev/github/test/node_modules/react-scripts/config/modules.js:74:14)
    at Object.<anonymous> (/Users/jack/dev/github/test/node_modules/react-scripts/config/modules.js:92:18)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
    at Module.load (internal/modules/cjs/loader.js:612:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
    at Function.Module._load (internal/modules/cjs/loader.js:543:3)
    at Module.require (internal/modules/cjs/loader.js:650:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (/Users/jack/dev/github/test/node_modules/react-scripts/config/webpack.config.js:31:17)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
    at Module.load (internal/modules/cjs/loader.js:612:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
    at Function.Module._load (internal/modules/cjs/loader.js:543:3)
error Command failed with exit code 1.

This PR uses typescript's readConfigFile which handles comments.

[heyimalex]

I'm not sure this is the right solution, as we also need to handle jsconfig.json. We probably don't want to have a hard dependency on typescript, so we already need to solve jsconfig parsing without it, and then whatever we use for that can also be used for ts.

I wonder exactly how nuanced the parsing is?

Related to #6865

[mrmckeb]

I think the solution is probably to read the files, instead of using require, and then parsing them.

Perhaps we need a library to help here. @crosscompile, TypeScript would only be installed when the project is TypeScript, so this solution wouldn't work for jsconfig.json files as @heyimalex mentioned.

[jackjocross]

I adapted this approach from verifyTypeScriptSetup. typescript is required inline after checking hasTsConfig so that it is only required when TypeScript is being used. Maybe it would also be useful to include these error messages.

I think a question is whether we want to use a single custom parser for tsconfig.json and jsconfig.json files, or typescript's parseJsonConfigFileContent for tsconfig.json and a custom parser for jsconfig.json. Once that is decided we could then update this PR and verifyTypeScriptSetup.

Is there a downside to using typescript's provided parseJsonConfigFileContent when TypeScript is being used?

[mrmckeb]

@crosscompile, I think what we're saying is that we would like a single approach for both. We could:

• a) find a package that can parse the JSON
• b) Create a util that can parse both (might be very easy anyway)
• c) Use TypeScript for both

The problem with "c" is that TypeScript would need to be installed at all times, and it's now only added for TS projects.

[jackjocross]

Ah got it, yes those seem like the three options. After doing a little research a few thoughts came up for each option:

a) find a package that can parse the JSON

In addition to parsing the JSON we also have to resolve the extends key recursively.

b) Create a util that can parse both (might be very easy anyway)

I tracked down the parseConfig code to help gauge the effort to replicate it: https://github.com/Microsoft/TypeScript/blob/0179d974031e311cee4fe37a19e61b1c0a8800d7/src/compiler/commandLineParser.ts#L1587-L1633

c) Use TypeScript for both

As an alternative to always installing typescript, in the presence of a jsconfig.json or tsconfig.json we could attempt to require typescript and display a helpful error if it's not installed. This is what verifyTypescriptSetup does now: https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/scripts/utils/verifyTypeScriptSetup.js#L59-L92.

I also had not used jsonfig.json and found this definition in the VS Code docs if anyone else finds it useful:

jsconfig.json is a descendant of tsconfig.json, which is a configuration file for TypeScript. jsconfig.json is tsconfig.json with "allowJs" attribute set to true.

[mrmckeb]

Correct, but you can use jsconfig.json without installing TypeScript - that's because VSCode has its own instance/version of TypeScript that it uses by default. So we can't throw an error at those users, because right now it works fine for them (sans-comments).

We also look at jsconfig.json and use the baseUrl from there in place of NODE_PATH.

With option A, we could use this: https://www.npmjs.com/package/comment-json. So, we'd read the file (sync) and then process with this to get the JSON we need. That should solve the issue you've reported, right?

[jackjocross]

Cool, yep that approach would solve the issue with comments in my config.

One issue I ran into when trying out comment-json is that it doesn't handle trailing commas, which some CRA users have run into (#6865). Maybe there is an alternative library that handles trailing commas.

tsconfig.json also supports an extends property which allows you to inherit configs, so technically to fully support reading tsconfig.json we would need to recursively follow the extends property. In my experience extends is not commonly used for baseUrl so ignoring extends here would work for me, but not sure about others.

[mrmckeb]

Honestly, I feel that JSON with comments should have had a mandatory file extension (like JSONC) as many tools aren't going to support it...

Your concerns about extends are valid. I think that's a good case to consider installing TypeScript as a part of react-scripts itself. What do you think @ianschmitz? The alternative would be to document that we only read path from the root config.

[heyimalex]

@mrmckeb My gut feeling is that adding a dependency on typescript is a bad choice. I'm just not sure how well multiple versions of typescript in the dependency tree play together, and I think pinning CRA to a particular version would be a mistake. But maybe it's fine?

Ideally we could rip out just the code we need, but I'm not sure anyone wants to go through the trouble or take on the maintenance burden.

We could use typescript if it's installed, but fall back to using require or JSON.parse, with an error message displayed on parse failure that says comments aren't supported unless typescript is installed. Additionally, if it does parse successfully but has an extends key, we could print a warning that extends isn't supported unless typescript is installed.

This:

• Works perfectly for 99% of people using CRA with typescript, since they already have ts installed.
• Puts very little burden on us, since we don't need to re-implement everything tsc does to parse json and resolve extends.
• Only effects the small subset of people using comments or trailing commas in jsconfig.json by making them add a dependency on typescript.

It's not a perfect solution, which maybe isn't the CRA way, but I can write the PR if you think it's a good path forward.

[Pajn]

Having multiple Typescript versions in the node_modules tree isn't a problem, I have used that setup in multiple projects. However, pinning CRA to a particular Typescript version would be problematic though as Typescript does not follow semver and Typescript version updates inside CRA can become a hassle.
If Typescript is bundled with CRA it would need to support overriding by the user, using the current method.

With a big project like CRA having this problem, maybe the TS team can be persuaded to split "readConfigFile" into its own package that both TS and CRA can depend upon. However while neither bundling Typescript (but still allowing override) and @heyimalex "switching parser" solutions is perfect, I think both is workable and the caveats are only inconvenient.

[mrmckeb]

Thanks @heyimalex, and good thoughts.

I've spoken to @ianschmitz about pinning TS before, as we know that a few of the dependencies (such as the ESLint parser) also pin/restrict versions... so that's why it might be an acceptable option. And as @Pajn said it should be fine. We'd need to test a little.

I actually also think that asking the TS team to split out the reader isn't a bad idea - it seems like something that should exist (JSON reader with comment support) given that so many people are using JSONC now.

That being said, it seems that Microsoft publishes this already - https://www.npmjs.com/package/jsonc-parser

Any thoughts @ianschmitz?

[jackjocross]

I like the idea of adding typescript as a dependency since tsconfig.json and jsconfig.json are both intended to be read with readConfigFile.

Since CRA would be using the typescript dependency only for reading config files and not type checking, there shouldn't be any issues with types breaking across minor typescript versions.

I also think it is very unlikely that readConfigFile will break across typescript versions since VSCode users using jsconfig.json would be negatively impacted as well.

If the TS team is open to splitting out readConfigFile that would be even better, but that implementation could be swapped in at a later time.

[mrmckeb]

OK, let's go with that approach @crosscompile.

I'll approve, but I need a second approval. @iansu might be able to take a look.

[mrmckeb]

Sorry @crosscompile, I just realised that this doesn't cover the jsconfig.json file - as @heyimalex mentioned. Are you able to add that to the PR? It's a few lines lower, and the fix should be identical.

If not, let me know and I can do it.

Still chasing a second approval on this ;)

[jackjocross]

@mrmckeb yep I can definitely make that change.

Do you also want me to add the typescript dependency to react-scripts?

[iansu]

Why are you using resolve.sync here?

[jackjocross]

I saw this approach being used in verifyTypeScriptSetup and thought that there must be a reason require isn't used directly.

I'm not 100% certain but it seems like it might have to do with how appDirectory is setup to resolve symlinks?

[Pajn]

I be leave this is necessary to support yarn pnp. As typescript, isn't a dependency of CRA, yarn would not let CRA require it directly. However, by resolving it from the app directory (which do have a dependency on typescript), CRA can still access it.

[mrmckeb]

Hey @crosscompile, I was talking to @iansu and we think we'll merge this in with TypeScript-only support, and consider how to support jsconfig.json without adding TS as a dependency of react-scripts.