feat(compiler): add support for compile-time required inputs by crisbeto · Pull Request #49304 · angular/angular

crisbeto · 2023-03-03T08:05:55Z

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.

goldens/public-api/core/index.md

Harpush · 2023-03-03T12:17:48Z

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 · 2023-03-03T16:01:02Z

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.

packages/compiler/src/render3/view/api.ts

packages/core/src/metadata/directives.ts

goldens/public-api/core/index.md

packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts

packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts

elgreco247 · 2023-03-03T20:18:18Z

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}.

packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts

packages/compiler-cli/test/ngtsc/host_directives_spec.ts

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 · 2023-03-15T10:20:43Z

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.

AndrewKushnir

Reviewed-for: public-api

alxhub

Reviewed-for: public-api

jessicajaniuk

reviewed-for: public-api

crisbeto · 2023-03-15T16:15:37Z

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

alxhub · 2023-03-15T23:59:27Z

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

…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.

…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 · 2023-03-17T11:15:36Z

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 · 2023-03-17T11:17:08Z

it was reverted for some internal issues pt., 17 mar 2023 o 12:15 Evgeniy OZ ***@***.***> napisał(a):

…

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 <https://github.com/input>({required: true}) in a directive or a component: error TS2345: Argument of type '{ required: boolean; }' is not assignable to parameter of type 'string'. — Reply to this email directly, view it on GitHub <#49304 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFKBQQNMNGZJ4BVTGIFY76TW4RBWNANCNFSM6AAAAAAVOJMFJU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

e-oz · 2023-03-17T11:17:36Z

thanks @kbrilla

angular-automatic-lock-bot · 2023-04-17T00:08:23Z

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.}

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 added this to the v16-candidates milestone Mar 3, 2023

angular-robot bot added the detected: feature PR contains a feature commit label Mar 3, 2023

crisbeto commented Mar 3, 2023

View reviewed changes

goldens/public-api/core/index.md Outdated Show resolved Hide resolved

crisbeto force-pushed the required-inputs branch from a01da1b to 758f88f Compare March 3, 2023 08:50

crisbeto requested a review from alxhub March 3, 2023 16:01

crisbeto marked this pull request as ready for review March 3, 2023 16:01

pullapprove bot requested a review from devversion March 3, 2023 16:01

crisbeto force-pushed the required-inputs branch from 758f88f to 5faa1ab Compare March 3, 2023 16:04

crisbeto removed the request for review from devversion March 3, 2023 16:06

alxhub approved these changes Mar 3, 2023

View reviewed changes

packages/compiler/src/render3/view/api.ts Outdated Show resolved Hide resolved

packages/core/src/metadata/directives.ts Outdated Show resolved Hide resolved

pullapprove bot requested review from AndrewKushnir, atscott and jessicajaniuk March 3, 2023 16:56

jelbourn reviewed Mar 3, 2023

View reviewed changes

goldens/public-api/core/index.md Outdated Show resolved Hide resolved

packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts Outdated Show resolved Hide resolved

packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts Outdated Show resolved Hide resolved

kbrilla reviewed Mar 4, 2023

View reviewed changes

packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts Outdated Show resolved Hide resolved

packages/compiler-cli/test/ngtsc/host_directives_spec.ts Outdated Show resolved Hide resolved

crisbeto force-pushed the required-inputs branch from 5faa1ab to 1117de3 Compare March 5, 2023 21:05

crisbeto mentioned this pull request Mar 6, 2023

refactor(compiler): required inputs prerequisite refactors #49333

Closed

crisbeto force-pushed the required-inputs branch 3 times, most recently from 4667917 to dfc59a9 Compare March 15, 2023 09:56

crisbeto force-pushed the required-inputs branch from dfc59a9 to 8d0da53 Compare March 15, 2023 11:00

AndrewKushnir approved these changes Mar 15, 2023

View reviewed changes

pullapprove bot requested a review from alxhub March 15, 2023 15:13

alxhub approved these changes Mar 15, 2023

View reviewed changes

pullapprove bot requested a review from alxhub March 15, 2023 15:34

alxhub approved these changes Mar 15, 2023

View reviewed changes

jessicajaniuk approved these changes Mar 15, 2023

View reviewed changes

crisbeto removed the request for review from atscott March 15, 2023 16:38

alxhub closed this in 1a6ca68 Mar 16, 2023

alxhub mentioned this pull request Mar 16, 2023

Revert "feat(compiler): add support for compile-time required inputs … #49449

Closed

Yogu mentioned this pull request Mar 16, 2023

Contradictory guidelines in Style 05-18 (strictPropertyInitialization) #46420

Closed

crisbeto mentioned this pull request Mar 16, 2023

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

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 17, 2023

Conversation

crisbeto commented Mar 3, 2023

Uh oh!

Uh oh!

Harpush commented Mar 3, 2023

Uh oh!

crisbeto commented Mar 3, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elgreco247 commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crisbeto commented Mar 15, 2023

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

alxhub left a comment

Choose a reason for hiding this comment

Uh oh!

jessicajaniuk left a comment

Choose a reason for hiding this comment

Uh oh!

crisbeto commented Mar 15, 2023

Uh oh!

alxhub commented Mar 15, 2023

Uh oh!

e-oz commented Mar 17, 2023

Uh oh!

kbrilla commented Mar 17, 2023 via email

Uh oh!

e-oz commented Mar 17, 2023

Uh oh!

angular-automatic-lock-bot bot commented Apr 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

elgreco247 commented Mar 3, 2023 •

edited

Loading