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

`strictTemplates` implies `fullTemplateTypeCheck` #34195

Open
wants to merge 4 commits into
base: master
from

Conversation

@JoostK
Copy link
Member

JoostK commented Dec 2, 2019

See individual commits.

@JoostK JoostK requested a review from angular/fw-compiler as a code owner Dec 2, 2019
@ngbot ngbot bot modified the milestone: needsTriage Dec 2, 2019
@googlebot googlebot added the cla: yes label Dec 2, 2019
@JoostK JoostK requested a review from IgorMinar Dec 2, 2019
*/
function verifyCompatibleTypeCheckOptions(options: api.CompilerOptions): void {
if (options.fullTemplateTypeCheck === false && options.strictTemplates === true) {
throw new Error(

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 5, 2019

Contributor

We could emit this as a diagnostic in getNgOptionDiagnostics. I've never done an option diagnostic though - might be worth a shot.

@alxhub
alxhub approved these changes Dec 5, 2019
@JoostK JoostK force-pushed the JoostK:ngtsc-ttc-option-compat branch 2 times, most recently from e08f2df to 9d1251e Dec 5, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Dec 5, 2019

Copy link
Member

IgorMinar left a comment

I suggest we clean up the docs a bit more. See individual commits.

thanks for making this change.

@@ -46,7 +46,8 @@ The following still have type `any`.
### Strict mode

Angular version 9 maintains the behavior of the `fullTemplateTypeCheck` flag, and introduces a third "strict mode".
Strict mode is accessed by setting both `fullTemplateTypeCheck` and the `strictTemplates` flag to `true`.
Strict mode is accessed by setting the `strictTemplates` flag to `true`, which implies that `fullTemplateTypeCheck` is also enabled.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Dec 6, 2019

Member

I'd simplify this to just say:

Suggested change
Strict mode is accessed by setting the `strictTemplates` flag to `true`, which implies that `fullTemplateTypeCheck` is also enabled.
Strict mode is the a superset of full mode, and is accessed by setting the `strictTemplates` flag to true. This flag supersedes the `fullTemplateTypeCheck` flag.
@@ -46,7 +46,8 @@ The following still have type `any`.
### Strict mode

Angular version 9 maintains the behavior of the `fullTemplateTypeCheck` flag, and introduces a third "strict mode".
Strict mode is accessed by setting both `fullTemplateTypeCheck` and the `strictTemplates` flag to `true`.
Strict mode is accessed by setting the `strictTemplates` flag to `true`, which implies that `fullTemplateTypeCheck` is also enabled.
It is therefore OK to not specify `fullTemplateTypeCheck` or to set it to `true`. Setting `fullTemplateTypeCheck` to `false` in this mode is an error.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Dec 6, 2019

Member

There is no need to talk about the error case. The error will be thrown when users use the settings incorrectly.

Suggested change
It is therefore OK to not specify `fullTemplateTypeCheck` or to set it to `true`. Setting `fullTemplateTypeCheck` to `false` in this mode is an error.
// Skip template type-checking if it's disabled.
if (this.options.ivyTemplateTypeCheck === false &&
this.options.fullTemplateTypeCheck !== true) {
if (this.options.ivyTemplateTypeCheck === false && !fullTemplateTypeCheck) {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Dec 6, 2019

Member

I find this condition very misleading and confusing. I always have even before this change.

How about:

const templateTypeCheck = (strictTemplates || !!this.options.fullTemplateTypeCheck) && this.options.ivyTemplateTypeCheck !== false;

if (!templateTypeCheck) {
  return [];
}

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 6, 2019

Author Member

You are not alone, it confuses me just as much as it confuses you :-)

Your suggestion, however, is not equivalent to the current logic. Currently, even if "ivyTemplateTypeCheck" is false will typechecking still be enabled if "fullTemplateTypeCheck"/"strictTemplates" is set. Your suggestion as I interpret it would not behave the same, as having "ivyTemplateTypeCheck" set to false would completely disable type-checking under all circumstances.

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 12, 2019

Contributor

I'm okay with the behavior change. If you're curious, the logic worked this way because fullTemplateTypeCheck is easy to control as an option of the ng_module build rule. So I could set ivyTemplateTypeCheck to false in bazel/g3, and then set fullTemplateTypeCheck via the ng_module option when I wanted to test type-checking in g3 projects.

This logic predates the strictTemplates though - now setting fullTemplateTypeCheck isn't enough. So I'm happy to change it.

One of the following actions is required:
1. Enable "fullTemplateTypeCheck" by setting it to 'true', or by removing the option entirely.
2. Disable "strictTemplates" by setting it to 'false', or by removing the option entirely.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Dec 6, 2019

Member

I don't think that it makes sense to encourage people to set both strictTemplates and fullTemplateTypeCheck in the config. Fewer flags is better, especially if we in the future want to remove the "fullTemplateTypeCheck" flag.

Can you please change this, so that the two options are:

  1. remove the "fullTemplateTypeCheck" option.
  2. remove "strictTemplates" or set it to "false".
@@ -245,7 +245,7 @@ export declare class AnimationEvent {
});

it('should check expressions and their type when overall strictness is enabled', () => {
env.tsconfig({fullTemplateTypeCheck: true, strictTemplates: true});
env.tsconfig({strictTemplates: true});

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Dec 6, 2019

Member

much better!!!

JoostK added 4 commits Dec 2, 2019
It is now an error if '"fullTemplateTypeCheck"' is disabled while
`"strictTemplates"` is enabled, as enabling the latter implies that the
former is also enabled.
Previously, it was required that both `fullTemplateTypeCheck` and
`strictTemplates` had to be enabled for strict mode to be enabled. This
is strange, as `strictTemplates` implies `fullTemplateTypeCheck`. This
commit makes setting the `fullTemplateTypeCheck` flag optional so that
strict mode can be enabled by just setting `strictTemplates`.
@JoostK JoostK force-pushed the JoostK:ngtsc-ttc-option-compat branch from 9d1251e to d918d11 Dec 6, 2019
@JoostK

This comment has been minimized.

Copy link
Member Author

JoostK commented Dec 6, 2019

@IgorMinar thanks for the review, your suggestions have been applied.

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