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

tsconfig.json with comments #2765

Closed
ConsoleTVs opened this issue Aug 12, 2019 · 4 comments

Comments

@ConsoleTVs
Copy link

commented Aug 12, 2019

The latest release on Deno panics when the tsconfig.json contains comments. While JSON is indeed invalid due to that reason, most people still uses it since it's how it comes by default on typescript.

deno --allow-net --config tsconfig.json server.ts
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: ErrBox(DenoError { kind: InvalidInput, msg: "Compiler config is not a valid JSON" })', src\libcore\result.rs:999:5
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
cat tsconfig.json
{
  "compilerOptions": {
    /* Basic Options */
    // "jsx": "preserve",                     /* Specify JSX code generation: 'preserve', 'react-native', or 'react'. */
    "removeComments": true,                   /* Do not emit comments to output. */

    /* Strict Type-Checking Options */
    "strict": true,                           /* Enable all strict type-checking options. */
    // "noImplicitAny": true,                 /* Raise error on expressions and declarations with an implied 'any' type. */
    // "strictNullChecks": true,              /* Enable strict null checks. */
    // "strictFunctionTypes": true,           /* Enable strict checking of function types. */
    // "strictBindCallApply": true,           /* Enable strict 'bind', 'call', and 'apply' methods on functions. */
    // "strictPropertyInitialization": true,  /* Enable strict checking of property initialization in classes. */
    // "noImplicitThis": true,                /* Raise error on 'this' expressions with an implied 'any' type. */
    // "alwaysStrict": true,                  /* Parse in strict mode and emit "use strict" for each source file. */

    /* Additional Checks */
    "noUnusedLocals": true,                   /* Report errors on unused locals. */
    // "noUnusedParameters": true,            /* Report errors on unused parameters. */
    "noImplicitReturns": true,                /* Report error when not all code paths in function return a value. */
    "noFallthroughCasesInSwitch": true,       /* Report errors for fallthrough cases in switch statement. */
  }
}

Keep in mind, the trailing comma also causes the issue while working fine on older versions.

@kevinkassimo

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

Serde json does not support comments: serde-rs/json#168
https://github.com/hjson/hjson-rust do and is mentioned in that issue (hjson is a superset of JSON), but it also comes with extra stuff which I'm not sure if we should adopt

@kitsonk

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

The only thing we need it for in Rust land is to detect "checkJs": true to know to cache JavaScript modules return. The TypeScript compiler supports this (and needs to parse the files directly anyways). There are two solutions I see:

  • Just regex out the flag, which won't be 100% accurate, but really would be an edge case.
  • Not have Rust determine this flag directly and change the compiler API to report this flag back. Which might be better in the long term, if there are more options that Rust needs to understand.
@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

The only thing we need it for in Rust land is to detect "checkJs": true to know to cache JavaScript modules return. The TypeScript compiler supports this (and needs to parse the files directly anyways). There are two solutions I see:

  • Just regex out the flag, which won't be 100% accurate, but really would be an edge case.
  • Not have Rust determine this flag directly and change the compiler API to report this flag back. Which might be better in the long term, if there are more options that Rust needs to understand.

First option is very easy to implement in current infrastructure. The second one is better but I see some challenges with it:

  • TS compiler is spawned lazily - ie. if file needs to be run through TS compiler it's spinned up
  • to know if file needs to be run through TS compiler we need to know which media types TS compiler supports (ie. "ts" and "js" if checkJs: true)

So we'd need to spin up TS compiler just to know what files to compile. I think that's out of question as it'll add a lot of overhead.

@kitsonk

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Ah yeah, I forgot about that. I think we would need to do 1 then, and while it won't always be 100% accurate, someone would have to really screw things up and there are more likely to encounter a parse error in the compiler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.