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

Rename TSConfig option categories #42514

Merged
merged 14 commits into from
Jun 3, 2021
Merged

Rename TSConfig option categories #42514

merged 14 commits into from
Jun 3, 2021

Conversation

orta
Copy link
Contributor

@orta orta commented Jan 27, 2021

We had a docs meeting about this at the end of 2020, which resulted in this set of categories

Screen Shot 2021-01-27 at 4 32 47 PM

The docs changes are mostly just trivial, but there is code in showConfig which relied on behavior in that any config option in advanced types would not show in certain circumstances. This is what I'm not 100% sure on, is this setup acceptable? The diff in tests is mostly just re-ordering.

@orta orta self-assigned this Jan 27, 2021
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 27, 2021
@DanielRosenwasser
Copy link
Member

I don't know how I feel about this change - I think we should strive to reduce tsconfig.json itself.

A lot of these are way more rare options that I would never suggest a new user think about:

  • jsxFactory
  • jsxFragmentFactory
  • jsxImportSource
  • rootDirs
  • typeRoots
  • noResolve
  • maxNodeModuleJsDepth
  • outFile
  • removeComments
  • downlevelIteration
  • useDefineForClassFields

@DanielRosenwasser
Copy link
Member

I guess I misread the change - looks like we're not adding stuff, just recategorizing. Still, I think long term we should try to pare this thing down.

src/compiler/commandLineParser.ts Show resolved Hide resolved
src/compiler/diagnosticMessages.json Outdated Show resolved Hide resolved
src/compiler/commandLineParser.ts Outdated Show resolved Hide resolved
"compilerOptions": {
"locale": "someString"
}
"compilerOptions": {}
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that it was shown before and not now..

Copy link
Contributor Author

@orta orta Jan 28, 2021

Choose a reason for hiding this comment

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

Yeah, locale used to be an Advanced Option which meant it would have passed that previous if check - but the flag only works on the CLI (and so it's not in the tsconfig reference for example), so I felt moving it into command line options was probably the right call

            name: "locale",
             type: "string",
-            category: Diagnostics.Advanced_Options,
+            category: Diagnostics.Command_line_Options,

Copy link
Member

Choose a reason for hiding this comment

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

It is not command line only option... it can be present in tsconfig ?

Copy link
Contributor Author

@orta orta May 28, 2021

Choose a reason for hiding this comment

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

Just confirmed to make sure, tsc ignores locale in a tsconfig

❯ cat tsconfig.json
{
  "files": [
    "broke.ts"
  ],
  "compilerOptions": {
    "locale": "cs",
  }
}

❯ pnpm tsc
broke.ts:2:7 - error TS1134: Variable declaration expected.

2 const 123 = 123
        ~~~


Found 1 error.


❯ pnpm tsc --locale cs
broke.ts:2:7 - error TS1134: Očekává se deklarace proměnné.

2 const 123 = 123
        ~~~


Našla se 1 chyba.

and just in case it's a top level attribute:

❯ cat tsconfig.json
{
  "files": [
    "broke.ts"
  ],
  "locale": "cs",
}

❯ pnpm tsc
broke.ts:2:7 - error TS1134: Variable declaration expected.

2 const 123 = 123
        ~~~


Found 1 error.

It can be considered a bug? but the switch at least makes sense for how TS acts today

@orta
Copy link
Contributor Author

orta commented Jan 28, 2021

Hah, yeah, you're a braver man than me Daniel to think about removing tsconfig entries! At least this change makes the list of deprecated flags longer

@orta
Copy link
Contributor Author

orta commented Apr 28, 2021

I've given this a rebase, and updated it from the 2 times it came up in language design meetings - should be good for another review @sheetalkamat ❤️

src/compiler/diagnosticMessages.json Show resolved Hide resolved
"compilerOptions": {
"locale": "someString"
}
"compilerOptions": {}
Copy link
Member

Choose a reason for hiding this comment

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

It is not command line only option... it can be present in tsconfig ?

@orta orta merged commit 9d345e7 into microsoft:main Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants