Skip to content

fix(text-field): correctly check _initialHeight #17900

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 10, 2019
Merged

fix(text-field): correctly check _initialHeight #17900

merged 1 commit into from
Dec 10, 2019

Conversation

@mprobst
Copy link
Contributor

@mprobst mprobst commented Dec 9, 2019

TypeScript 3.7 flags the assignment on line 250 because style.height must not be null.

The underlying reason though appears to be that the comparison checks against === undefined, where the field only declares null as an option. The field is not initialized, so undefined is the correct value to compare to and type to use.

@mprobst mprobst requested a review from mmalerba as a code owner December 9, 2019 13:18
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 9, 2019
@mprobst
fix(text-field): correctly check _initialHeight
57399ac 
TypeScript 3.7 flags the assignment on line 250 because `style.height`
must not be `null`.

The underlying reason though appears to be that the comparison checks
against `=== undefined`, where the field only declares `null` as an
option, thus TS complains that the value might still be `null`.

This change just spills a bunch of `any` over the code to get it to
compile in both 3.6 and 3.7. The casts shoulb be removed once the code
is migrated.
crisbeto
crisbeto reviewed Dec 9, 2019
View reviewed changes
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.

The changes LGTM, but it looks like it broke one of the tests.

@mprobst
Copy link
Contributor Author

mprobst commented Dec 9, 2019

@crisbeto ack, I apparently pushed a fix to the wrong remote branch. Should be good now I hope.

crisbeto
crisbeto approved these changes Dec 9, 2019
View reviewed changes
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

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Dec 9, 2019
@crisbeto
Copy link
Member

crisbeto commented Dec 9, 2019

Marking as merge safe because it only changes the type of a private property to any so there's no chance of it introducing compilation errors for consumers.

@mmalerba mmalerba merged commit a3e953e into master Dec 10, 2019
@Splaktar Splaktar deleted the initialHeight branch January 6, 2020 04:49
@Splaktar
Copy link
Contributor

Splaktar commented Jan 6, 2020

FYI, in the future, the correct commit message would be

fix(cdk/text-field): correctly check _initialHeight

Due to this commit message issue, the release notes include this change in material instead of cdk (which is empty).

@Splaktar Splaktar added this to the 9.0.0 milestone Jan 6, 2020
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Feb 6, 2020

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 Feb 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@crisbeto crisbeto crisbeto approved these changes

@mmalerba mmalerba Awaiting requested review from mmalerba

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@mprobst @crisbeto @Splaktar @googlebot @mmalerba