Skip to content

fix(slider): displayWith function never called with "null" #16707

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

Conversation

devversion
Copy link
Member

@devversion devversion commented Aug 7, 2019

Currently the "displayWith" function for a MatSlider is typed
in a way that implies that the function is sometimes called with
null as value. This is never the case because the value property
of the slider always returns a number but is just typed as null-able
because getters and setters cannot have different types.

This fixes the type for the displayWith function as it will never be
called with null.

@jelbourn Not sure what the best target for this PR would be. It's technically a fix but I could also see this as breaking for applications with strictFunctionTypes and Ivy's strict input type checking enabled

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 7, 2019
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.

It's been a while since I added that input, but from what I remember, the case when the value can be null is when the ControlValueAccessor associated with the slider is reset. There might be something down the line that coerces it to 0 though.

@devversion
Copy link
Member Author

@crisbeto Yeah unless I miss something, the getter that returns the value falls back to the minimum value if the value is null. The minimum value is always a number (with fallback of 0).

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

If this doesn't break anything in Google we can consider it safe for a patch release

@jelbourn jelbourn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release P4 A relatively minor issue that is not relevant to core functions labels Aug 14, 2019
@jelbourn
Copy link
Member

Caretaker note: if the API change breaks anything in Google, bump this PR to target: major

@andrewseguin andrewseguin added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Aug 21, 2019
@andrewseguin
Copy link
Contributor

Presubmits passed, can be merged as patch. Just needs rebase

Currently the "displayWith" function for a `MatSlider` is typed
in a way that implies that the function is sometimes called with
null as value. This is never the case because the `value` property
of the slider always returns a number but is just typed as `null`-able
because getters and setters cannot have different types.

This fixes the type for the `displayWith` function as it will never be
called with `null`.
@devversion devversion force-pushed the fix/slider-display-with-function-never-null branch from 1b0ace4 to d4a2d2d Compare August 21, 2019 20:54
@devversion
Copy link
Member Author

@andrewseguin Rebased.

@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Aug 21, 2019
@ngbot
Copy link

ngbot bot commented Aug 22, 2019

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure status "ci/circleci: tests_browserstack" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@jelbourn jelbourn merged commit 17c8983 into angular:master Aug 27, 2019
mmalerba pushed a commit to mmalerba/components that referenced this pull request Aug 27, 2019
…6707)

Currently the "displayWith" function for a `MatSlider` is typed
in a way that implies that the function is sometimes called with
null as value. This is never the case because the `value` property
of the slider always returns a number but is just typed as `null`-able
because getters and setters cannot have different types.

This fixes the type for the `displayWith` function as it will never be
called with `null`.
@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 27, 2019
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 merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note P4 A relatively minor issue that is not relevant to core functions target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants