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

perf(core): be more consistent about typeof checks #28400

Closed
wants to merge 1 commit into from

Conversation

@bmeurer
Copy link
Contributor

bmeurer commented Jan 28, 2019

When testing whether value is an object, use the ideal sequence of
strictly not equal to null followed by typeof value === 'object'
consistently. Specifically there's no point in using double equal with
null since undefined is ruled out by the typeof check.

Also avoid the unnecessary ToBoolean check on value.ngOnDestroy in
hasOnDestroy(), since the typeof value.ngOnDestroy === 'function'
will only let closures pass and all closures are truish (with the
notable exception of document.all, but that shouldn't be relevant
for the ngOnDestroy hook).

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

Does this PR introduce a breaking change?

  • Yes
  • No
perf(core): be more consistent about typeof checks
When testing whether `value` is an object, use the ideal sequence of
strictly not equal to `null` followed by `typeof value === 'object'`
consistently. Specifically there's no point in using double equal with
`null` since `undefined` is ruled out by the `typeof` check.

Also avoid the unnecessary ToBoolean check on `value.ngOnDestroy` in
`hasOnDestroy()`, since the `typeof value.ngOnDestroy === 'function'`
will only let closures pass and all closures are truish (with the
notable exception of `document.all`, but that shouldn't be relevant
for the `ngOnDestroy` hook).

@bmeurer bmeurer requested a review from angular/fw-core as a code owner Jan 28, 2019

@googlebot googlebot added the cla: yes label Jan 28, 2019

@ngbot ngbot bot added this to the needsTriage milestone Jan 29, 2019

@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource left a comment

LGTM, thnx. Please keep those PRs coming, much appreciated!

@pkozlowski-opensource

This comment has been minimized.

Copy link
Member

pkozlowski-opensource commented Jan 29, 2019

merge-assistance as it needs g3 presubmit

@jasonaden jasonaden closed this in 9af18c2 Jan 29, 2019

@bmeurer bmeurer deleted the bmeurer:perf_typeof_object branch Jan 30, 2019

@bmeurer

This comment has been minimized.

Copy link
Contributor Author

bmeurer commented Jan 30, 2019

Thanks 👍

vetom added a commit to vetom/angular that referenced this pull request Jan 31, 2019

perf(core): be more consistent about typeof checks (angular#28400)
When testing whether `value` is an object, use the ideal sequence of
strictly not equal to `null` followed by `typeof value === 'object'`
consistently. Specifically there's no point in using double equal with
`null` since `undefined` is ruled out by the `typeof` check.

Also avoid the unnecessary ToBoolean check on `value.ngOnDestroy` in
`hasOnDestroy()`, since the `typeof value.ngOnDestroy === 'function'`
will only let closures pass and all closures are truish (with the
notable exception of `document.all`, but that shouldn't be relevant
for the `ngOnDestroy` hook).

PR Close angular#28400
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.