Skip to content

Add utility function to check for strict option flags #19427

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

Merged
merged 5 commits into from
Oct 30, 2017
Merged

Conversation

mhegazy
Copy link
Contributor

@mhegazy mhegazy commented Oct 23, 2017

Follow up on #19390, specifically: https://github.com/Microsoft/TypeScript/pull/19390/files#r146092916.

This change adds a new helper function to access strict flags consistently by checking their value then the --strict flag value if they are not set. it also:

  • Correctly check for noImplicitAny in checker
  • Correctly check for noImplicitAny in installTypesForPackage refactor

- Correctelly check for noImplicitAny in checker
- Correctelly check for noImplicitAny in installTypesForPackage refactor
@@ -1710,6 +1710,10 @@ namespace ts {
return moduleResolution;
}

export function getStrictOptionValue(compilerOptions: CompilerOptions, flag: "noImplicitAny" | "noImplicitThis" | "strictNullChecks" | "strictFunctionTypes" | "alwaysStrict"): boolean {
Copy link
Member

@weswigham weswigham Oct 23, 2017

Choose a reason for hiding this comment

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

Is there some way we can brand CompilerOptions internally in the type system such that we cannot use the strict-depending fields directly - they must be accessed through this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could not think of a way to do that without changing the shape of the compiler options object..

Copy link
Member

Choose a reason for hiding this comment

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

Documenting exploration for future reference: You can move members into a superclass/superinterface, then override them (marked @internal) with an intersection with never to change the field type to never internally (yet this is not an error for very odd reasons) - however in practice this messes with unit tests (some of which expect to explicitly set fields of those types) and doesn't prevent you from doing things like saying if (compilerOptions.noImplicitAny), which was the goal (as a never member is still considered present) :(

Copy link
Member

@ahejlsberg ahejlsberg left a comment

Choose a reason for hiding this comment

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

Approved with the suggested change.

@@ -1710,6 +1710,10 @@ namespace ts {
return moduleResolution;
}

export function getStrictOptionValue(compilerOptions: CompilerOptions, flag: "noImplicitAny" | "noImplicitThis" | "strictNullChecks" | "strictFunctionTypes" | "alwaysStrict"): boolean {
Copy link
Member

@ahejlsberg ahejlsberg Oct 24, 2017

Choose a reason for hiding this comment

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

I suggest declaring and using the following type for the flag property:

type StrictOptionName = "noImplicitAny" | "noImplicitThis" | "strictNullChecks" | "strictFunctionTypes" | "alwaysStrict";

@@ -3676,6 +3676,8 @@ namespace ts {

export type CompilerOptionsValue = string | number | boolean | (string | number)[] | string[] | MapLike<string[]> | PluginImport[] | null | undefined;

export type StrictOptionName = "noImplicitAny" | "noImplicitThis" | "strictNullChecks" | "strictFunctionTypes" | "alwaysStrict";

Copy link
Member

Choose a reason for hiding this comment

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

Can this not be in core (since the function itself is internal why not type as well be internal)

@mhegazy mhegazy merged commit 6c71ca8 into master Oct 30, 2017
@mhegazy mhegazy deleted the FollowUpFor19390 branch October 30, 2017 20:05
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants