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

feat(compiler): add support for compile-time required inputs #49304

Closed
wants to merge 1 commit into from

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
packages/compiler/src/render3/view/api.ts Outdated Show resolved Hide resolved
packages/core/src/metadata/directives.ts Outdated Show resolved Hide resolved
@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}.

crisbeto added a commit to crisbeto/angular that referenced this pull request Mar 6, 2023
Based on the discussion in angular#49304 (comment). Reworks how inputs are stored throughout the compiler to allow for required information to be stored alongside them. The refactor introduces a new `ClassInputPropertyMapping` class which is similar to the existing `ClassPropertyMapping`, but with the ability to store additional information about the mapping which may be handy for future features as well.
crisbeto added a commit to crisbeto/angular that referenced this pull request Mar 6, 2023
Based on the discussion in angular#49304 (comment). Reworks how inputs are stored throughout the compiler to allow for required information to be stored alongside them. The refactor introduces a new `ClassInputPropertyMapping` class which is similar to the existing `ClassPropertyMapping`, but with the ability to store additional information about the mapping which may be handy for future features as well.
crisbeto added a commit to crisbeto/angular that referenced this pull request Mar 7, 2023
Based on the discussion in angular#49304 (comment). Reworks how inputs are stored throughout the compiler to allow for required information to be stored alongside them. The refactor introduces a new `ClassInputPropertyMapping` class which is similar to the existing `ClassPropertyMapping`, but with the ability to store additional information about the mapping which may be handy for future features as well.
crisbeto added a commit to crisbeto/angular that referenced this pull request Mar 7, 2023
Based on the discussion in angular#49304 (comment). Reworks how inputs are stored throughout the compiler to allow for required information to be stored alongside them. The refactor introduces a new `ClassInputPropertyMapping` class which is similar to the existing `ClassPropertyMapping`, but with the ability to store additional information about the mapping which may be handy for future features as well.
crisbeto added a commit to crisbeto/angular that referenced this pull request Mar 7, 2023
Based on the discussion in angular#49304 (comment). Reworks how inputs are stored throughout the compiler to allow for required information to be stored alongside them. The refactor introduces a new `ClassInputPropertyMapping` class which is similar to the existing `ClassPropertyMapping`, but with the ability to store additional information about the mapping which may be handy for future features as well.
crisbeto added a commit to crisbeto/angular that referenced this pull request Mar 7, 2023
Based on the discussion in angular#49304 (comment). Reworks how inputs are stored throughout the compiler to allow for required information to be stored alongside them. The refactor introduces a new `ClassInputPropertyMapping` class which is similar to the existing `ClassPropertyMapping`, but with the ability to store additional information about the mapping which may be handy for future features as well.
crisbeto added a commit to crisbeto/angular that referenced this pull request Mar 13, 2023
Based on the discussion in angular#49304 (comment). Reworks the compiler internals to allow for additional information about inputs to be stored. This is a prerequisite for required inputs.
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