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

Post strictPropertyInitialization flag flip cleanup #24571

Open
rkirov opened this Issue Jun 18, 2018 · 3 comments

Comments

Projects
None yet
4 participants
@rkirov
Contributor

rkirov commented Jun 18, 2018

I'm submitting a...

[x] Other... Please describe: Tracking issue for internal cleanup.

What needs to be done

In order to quickly turn on strictPropertyInitialization flag in the whole code base, we introduced ! on every non-initialized class field. Each one of those fields needs to be individually checked and decided whether:

  • the field should be initialized. Thus foo!: string, should become foo = 'default'.
  • the field should be marked as optional. Thus foo!: string, should become foo?: string.
  • the field is reliably initialized, but TS cannot infer that (initialization can occur outside the ctor, for example), so ! should be kept.
@alan-agius4

This comment has been minimized.

Contributor

alan-agius4 commented Jul 3, 2018

@rkirov when an input is required, would you mark with a band !? As in my option in you don't know that the consumer will provide that value, thus, a component should be doing check if that value has been actually provided or not. ! should only be used if the value is initialised such as on ngOnInit. that said though, if one used that component using the viewChild etc... the property will be marked that it will never be undefined. Which in some cases it might be wrong, based on which lifecycle hook the value is initialized in.

And seeing the comment from @mhevery in #21571 it seems that we are on the same page;

Nothing forces @input() to be set. A user of the your component may simply chose not to bind to that input in which case your @input() is not initialize and you should have checks for non-initialized case. I know we disagree, but I think this is the correct behavior.

Truth to the told I am not a particular fan of using the !

@rkirov

This comment has been minimized.

Contributor

rkirov commented Jul 12, 2018

If you want to have required semantics for your Inputs what we have been recommending is ! and an assert in ngOnInit. If are to be pedantic the assert should be in ngOnChanges, but that might be too much for some.

The general advice for ! in any place (on expressions or in field declarations names) is that it is unsafe and to be avoided unless there is a compelling reason.

I have an additional advice on cleaning up after my change specifically for angular components.

For Angular components

For angular components, use the following rules in deciding between:
a) adding initializer
b) make the field optional
c) leave the '!'

  • If the field is annotated with @input - Make the field optional b) or add an initializer a).
  • If the input is required for the component user - add an assertion in ngOnInit and apply c.
  • If the field is annotated @ViewChild, @ContentChild - Make the field optional b).
  • If the field is annotated with @ViewChildren or @ContentChildren - Add back '!' - c).
  • Fields that have an initializer, but it lives in ngOnInit. - Move the initializer to the constructor.
  • Fields that have an initializer, but it lives in ngOnInit and cannot be moved because it depends on other @input fields - Add back '!' - c).
@alan-agius4

This comment has been minimized.

Contributor

alan-agius4 commented Jul 12, 2018

Thanks for the detailed explanation much appreciated.

I quite like what you suggest above.

@ngbot ngbot bot modified the milestones: needsTriage, Backlog Jul 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment