Skip to content

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Mar 3, 2023

Adds support for marking a directive input as required. During template type checking, the compiler will verify that all required inputs have been specified and will raise a diagnostic if one or more are missing. Some specifics:

  • Inputs are marked as required by passing an object literal with a required: true property to the Input decorator or into the inputs array.
  • Required inputs imply that the directive can't work without them. This is why there's a new check that enforces that all required inputs of a host directive are exposed on the host.
  • Required input diagnostics are reported through the OutOfBandDiagnosticRecorder, rather than generating a new structure in the TCB, because it allows us to provide a better error message.
  • Currently required inputs are only supported during AOT compilation, because knowing which bindings are present during JIT can be tricky and may lead to increased bundle sizes.

Fixes #37706.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release area: compiler Issues related to `ngc`, Angular's template compiler labels Mar 3, 2023
@crisbeto crisbeto added this to the v16-candidates milestone Mar 3, 2023
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Mar 3, 2023
@Harpush
Copy link

Harpush commented Mar 3, 2023

How will it affect strictNullChecks flag? I believe we still need the ! operator? So instead of runtime checks in ngOnInit we can have the compiler throw errors for us. Anyway that's great! Thanks!

@crisbeto
Copy link
Member Author

crisbeto commented Mar 3, 2023

Note for reviewer: the test_aio_local failure was triggered by these changes, but it's a pre-existing issue. It'll be resolved by #49293.

@crisbeto crisbeto requested a review from alxhub March 3, 2023 16:01
@crisbeto crisbeto marked this pull request as ready for review March 3, 2023 16:01
@pullapprove pullapprove bot requested a review from devversion March 3, 2023 16:01
@crisbeto crisbeto removed the request for review from devversion March 3, 2023 16:06
@elgreco247
Copy link

elgreco247 commented Mar 3, 2023

  • Required inputs imply that the directive can't work without them. This is why there's a new check that enforces that all required inputs of a host directive are exposed on the host.

As described by @nartc in this tweet (https://twitter.com/Nartc1410/status/1631694281298640896?s=20) enforcing the exposure of required inputs of host directives might be a little bit too rigid concerning the reusability of a directive.

Maybe the configuration object of host directives can be augmented with a property specifying whether the exposure of required inputs should be enforced.

For example {directive: HostDir, inputs: ['inputAlias'], enforceRequiredInputs: false}.

alxhub pushed a commit that referenced this pull request Mar 14, 2023
Based on the discussion in #49304 (comment). Reworks the compiler internals to allow for additional information about inputs to be stored. This is a prerequisite for required inputs.

PR Close #49333
@crisbeto crisbeto force-pushed the required-inputs branch 3 times, most recently from 4667917 to dfc59a9 Compare March 15, 2023 09:56
@crisbeto
Copy link
Member Author

I've integrated the changes from #49333 and addressed the remaining feedback.

Adds support for marking a directive input as required. During template type checking, the compiler will verify that all required inputs have been specified and will raise a diagnostic if one or more are missing. Some specifics:
* Inputs are marked as required by passing an object literal with a `required: true` property to the `Input` decorator or into the `inputs` array.
* Required inputs imply that the directive can't work without them. This is why there's a new check that enforces that all required inputs of a host directive are exposed on the host.
* Required input diagnostics are reported through the `OutOfBandDiagnosticRecorder`, rather than generating a new structure in the TCB, because it allows us to provide a better error message.
* Currently required inputs are only supported during AOT compilation, because knowing which bindings are present during JIT can be tricky and may lead to increased bundle sizes.

Fixes angular#37706.
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from alxhub March 15, 2023 15:13
@pullapprove pullapprove bot requested a review from alxhub March 15, 2023 15:34
Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 15, 2023
@crisbeto
Copy link
Member Author

Carertaker note: one target is failing internally. It doesn't look related to this change.

@crisbeto crisbeto removed the request for review from atscott March 15, 2023 16:38
@alxhub
Copy link
Member

alxhub commented Mar 15, 2023

This PR was merged into the repository by commit 1a6ca68.

@alxhub alxhub closed this in 1a6ca68 Mar 16, 2023
alxhub added a commit to alxhub/angular that referenced this pull request Mar 16, 2023
…ngular#49304)"

This reverts commit 1a6ca68.

This breaks tests in google3 which might be depending on private APIs. We
need to update these tests before we can land this PR.
alxhub added a commit that referenced this pull request Mar 16, 2023
…49304)" (#49449)

This reverts commit 1a6ca68.

This breaks tests in google3 which might be depending on private APIs. We
need to update these tests before we can land this PR.

PR Close #49449
@e-oz
Copy link

e-oz commented Mar 17, 2023

Are you sure it's merged into 16.0.0-next.3?
I can't find it in 16.0.0-next.3. When I use @input({required: true}) in a directive or a component:

error TS2345: Argument of type '{ required: boolean; }' is not assignable to parameter of type 'string'.

@kbrilla
Copy link

kbrilla commented Mar 17, 2023 via email

@e-oz
Copy link

e-oz commented Mar 17, 2023

thanks @kbrilla

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler detected: feature PR contains a feature commit merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Canonical: Marking @Input as required
10 participants