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

Add support for raising if you set a tsconfig entry of target/module with the right setting in the root #44964

Merged
merged 5 commits into from Sep 8, 2021

Conversation

orta
Copy link
Contributor

@orta orta commented Jul 9, 2021

Fixes #43293

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

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

This seems like a nice addition, but why limit it to target and module? Surely we have an array of all valid or common option names somewhere, right?

@sandersn sandersn added this to Not started in PR Backlog Jul 15, 2021
@orta
Copy link
Contributor Author

orta commented Jul 19, 2021

Just wanted to do the least to avoid this wrath by using what is probably always gonna be set there:

Other tools are allowed to use the top-level keys here, and do, so there's some danger of compat problems if in the future we add a compiler setting with the same name as a tool's setting

@andrewbranch
Copy link
Member

What if you detect all compilerOptions keys in the root, but only issue an error if there is no compilerOptions object present at all? That way, third party tools could add any root key they want, even ones that share names with compilerOptions keys, as long as the user also added "compilerOptions": {}.

@orta
Copy link
Contributor Author

orta commented Jul 19, 2021

This idea is great, I'll switch it to that

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

All suggestions are for lazy array allocation

src/compiler/commandLineParser.ts Outdated Show resolved Hide resolved
src/compiler/commandLineParser.ts Outdated Show resolved Hide resolved
src/compiler/commandLineParser.ts Outdated Show resolved Hide resolved
PR Backlog automation moved this from Not started to Needs merge Jul 20, 2021
Co-authored-by: Andrew Branch <andrewbranch@users.noreply.github.com>
Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

Would be nice to just avoid the linter suppression.

Comment on lines 2920 to 2921
// eslint-disable-next-line no-in-operator
if (rootCompilerOptions && json && !("compilerOptions" in json)) {
Copy link
Member

Choose a reason for hiding this comment

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

You could write json.compilerOptions === undefined and avoid the linter suppression, right?

Copy link
Member

Choose a reason for hiding this comment

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

There’s also a hasProperty util

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>
@orta
Copy link
Contributor Author

orta commented Sep 7, 2021

OK, I've had a few looks over this will merge it at end of day

@orta orta merged commit 32168ed into microsoft:main Sep 8, 2021
PR Backlog automation moved this from Needs merge to Done Sep 8, 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
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Provide warnings/errors when putting tsconfig compilerOptions in the root of the tsconfig.json
4 participants