Skip to content

fix: add non-null assertions for non-nullable input bindings #20136

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

Merged
merged 1 commit into from
Aug 5, 2020

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Jul 29, 2020

Due to a bug in the template type checker, binding expressions that include
undefined in their type which are bound to directive inputs that don't accept
undefined would not be reported as an error, if the directive declares at
least one coercion member using ngAcceptInputType. angular/angular#38273
fixes this bug, which uncovers some template issues.

Long-term, it would be best if the directives would be updated to include
undefined in an input's type as appropriate. This would require a larger
effort and is likely to introduce some breaking changes to public API.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 29, 2020
Due to a bug in the template type checker, binding expressions that include
`undefined` in their type which are bound to directive inputs that don't accept
`undefined` would not be reported as an error, if the directive declares at
least one coercion member using `ngAcceptInputType`. angular/angular#38273
fixes this bug, which uncovers some template issues.

Long-term, it would be best if the directives would be updated to include
`undefined` in an input's type as appropriate. This would require a larger
effort and is likely to introduce some breaking changes to public API.
@JoostK JoostK marked this pull request as ready for review July 30, 2020 16:32
@JoostK JoostK requested a review from jelbourn July 30, 2020 16:35
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM. Marking as safe since it's only syntax-level changes in our own templates.

@crisbeto crisbeto added lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jul 30, 2020
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn
Copy link
Member

Going to do a presubmit for this one anyway out of an abundance of caution

@mmalerba mmalerba added target: minor This PR is targeted for the next minor release and removed lgtm target: patch This PR is targeted for the next patch release labels Jul 31, 2020
@mmalerba mmalerba merged commit 09e68db into angular:master Aug 5, 2020
JoostK added a commit to JoostK/angular that referenced this pull request Aug 5, 2020
The changes in angular/components#20136 are required to allow the
framework tests to succeed.
AndrewKushnir pushed a commit to angular/angular that referenced this pull request Aug 6, 2020
The changes in angular/components#20136 are required to allow the
framework tests to succeed.

PR Close #38273
JoostK added a commit to JoostK/angular that referenced this pull request Aug 7, 2020
…#38273)

The changes in angular/components#20136 are required to allow the
framework tests to succeed.

PR Close angular#38273
AndrewKushnir pushed a commit to angular/angular that referenced this pull request Aug 7, 2020
…#38378)

The changes in angular/components#20136 are required to allow the
framework tests to succeed.

PR Close #38273

PR Close #38378
@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 Sep 5, 2020
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 cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants