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

fix(cem-expanded-types): pass correct options to ts.convertCompilerOptionsFromJson #110

Merged
merged 1 commit into from
May 9, 2024

Conversation

maxpatiiuk
Copy link
Contributor

When debugging cem-expanded-types, I noticed that this line is called with incorrect argument:

const compilerOptions = ts.convertCompilerOptionsFromJson(config, ".");

The convertCompilerOptionsFromJson function is being called with config, but should be called with config.compilerOptions instead.

At present, ts.convertCompilerOptionsFromJson returns these errors since it receives incorrect config object:

[
  {
    "messageText": "Unknown compiler option '$schema'.",
    "category": 1,
    "code": 5023
  },
  {
    "messageText": "Unknown compiler option 'extends'.",
    "category": 1,
    "code": 5023
  },
  {
    "messageText": "Unknown compiler option 'include'.",
    "category": 1,
    "code": 5023
  },
  {
    "messageText": "Unknown compiler option 'files'.",
    "category": 1,
    "code": 5023
  },
  {
    "messageText": "Unknown compiler option 'compilerOptions'.",
    "category": 1,
    "code": 5023
  }
]

@break-stuff
Copy link
Owner

Will this differ based on the TS version teams are using?

@maxpatiiuk
Copy link
Contributor Author

Hmm, doesn't seem so

I.e looking at the git blame in the typescript codebase, they have been calling convertCompilerOptionsFromJson with configJson.config.compilerOptions in this file for the last 7 years:

https://github.com/microsoft/TypeScript/blob/bc14459f3b6af5a9547631a590fdfc0fdf35d042/src/harness/fourslashImpl.ts#L332

git blame:

https://github.com/microsoft/TypeScript/blame/bc14459f3b6af5a9547631a590fdfc0fdf35d042/src/harness/fourslashImpl.ts#L332

@break-stuff break-stuff merged commit c10b7a9 into break-stuff:main May 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants