fix(core): throw error message when @Output not initialized by aldo-roman · Pull Request #19116 · angular/angular

aldo-roman · 2017-09-08T23:43:47Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[X] 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:

What is the current behavior?

Listing an event in a directive without creating an associated EventEmitter instance results in cryptic error.

Issue Number: #3664

What is the new behavior?

Better error messages when an @Output property is not initialized.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

googlebot · 2017-09-08T23:43:51Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

aldo-roman · 2017-09-08T23:47:42Z

I signed it!

googlebot · 2017-09-08T23:47:44Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

aldo-roman · 2017-09-08T23:49:47Z

I signed it! (again)

googlebot · 2017-09-08T23:49:49Z

CLAs look good, thanks!

cedvdb · 2017-09-10T19:26:35Z

Yes, thank you! This thing always gets me

ngbot · 2018-01-16T15:38:47Z

Hello? Don't want to hassle you. Sure you're busy. But this PR has some merge conflicts that you probably ought to resolve.
That is... if you want it to be merged someday...

splincode · 2018-07-26T14:59:32Z

@mhevery Will this pull request be accepted?

ngbot · 2018-07-27T18:22:49Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     missing required status "code-review/pullapprove"
     status "ci/circleci: test_ivy_jit" is pending
     status "ci/circleci: build-packages-dist" is pending
     status "ci/circleci: test_ivy_aot" is pending
     status "ci/circleci: lint" is pending
     status "ci/circleci: test" is pending
     status "google3" is pending
     status "continuous-integration/travis-ci/pr" is pending

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.

googlebot · 2018-07-27T21:10:49Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-07-27T21:11:30Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

mhevery · 2018-07-27T22:19:20Z

http://test/OCL:206382419:BASE:206382467:1532729941989:b520bbc3

vicb · 2018-07-28T00:17:51Z

You should explicitely test for not !== null
(ie typeof null === 'object')

@vicb fixup (correct detect isObject typeof):

if (outputObservable === Object(outputObservable)) {

@splincode did you forget to push the change to the branch? I don't see the fix here. Please check.

oh, I see. @splincode you are not the PR owner. Do I understand correctly that you were just proposing that as the correct fix?

@IgorMinar Yes, exactly, could you make fixup it?

According to MDN:

The Object constructor creates an object wrapper for the given value. If the value is null or undefined, it will create and return an empty object, otherwise, it will return an object of a Type that corresponds to the given value. If the value is an object already, it will return the value.

As we do not want to create a new Object, (obs !== null && typeof obs === 'object') is probably the best ? (arguably we would only create a new object for invalid values)

@vicb An observable doesn't necessarily be a class instance, but can also be a function (WIP in RxJS v7 for performance consideration), and due to bad JavaScript design typeof a function is not 'object'. Possibly just check not null (!= null instead of !== null) as the issue is for "not initialized".

@trotyl Can I just do it this way?

const outputIsObservable = outputObservable && outputObservable.subscribe; if (outputIsObservable) { // ... }

or simplify

if (outputObservable instanceof Observable) { // ... }

@splincode It's already there, no need to duplicate:

angular/packages/core/src/util/lang.ts

Lines 23 to 26 in 3f20a2f

export function isObservable(obj: any | Observable<any>): obj is Observable<any> {

// TODO: use Symbol.observable when https://github.com/ReactiveX/rxjs/issues/2415 will be resolved

return !!obj && typeof obj.subscribe === 'function';

}

@vicb @trotyl so, simplify

if (isObservable(outputObservable)) { ... }

googlebot · 2018-08-01T23:07:30Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-08-01T23:17:06Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

googlebot · 2018-08-02T00:02:58Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

Closes angular#3664

googlebot · 2018-08-02T15:35:09Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

PR Close #19116

angular-automatic-lock-bot · 2019-09-13T21:52:16Z

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

googlebot added the cla: no label Sep 8, 2017

googlebot added cla: yes and removed cla: no labels Sep 8, 2017

jasonaden added the area: core Issues related to the framework runtime label Nov 14, 2017

aldo-roman force-pushed the fix/uninitialized-output-error branch 2 times, most recently from 4800acc to 7b3c139 Compare January 16, 2018 17:31

aldo-roman mentioned this pull request Jan 16, 2018

Better error messages when an @Output property is not initialized. #3664

Closed

mhevery approved these changes Jul 27, 2018

View reviewed changes

mhevery added the target: major This PR is targeted for the next major release label Jul 27, 2018

mhevery force-pushed the fix/uninitialized-output-error branch from 7b3c139 to 51ef8a0 Compare July 27, 2018 18:22

mhevery added the action: merge The PR is ready for merge by the caretaker label Jul 27, 2018

googlebot added cla: no and removed cla: yes labels Jul 27, 2018

mhevery added cla: yes and removed cla: no labels Jul 27, 2018

vicb reviewed Jul 28, 2018

View reviewed changes

vicb added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 28, 2018

googlebot added cla: no and removed cla: yes labels Aug 1, 2018

IgorMinar added cla: yes and removed cla: no labels Aug 1, 2018

IgorMinar removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Aug 1, 2018

googlebot added cla: no and removed cla: yes labels Aug 2, 2018

Aldo Roman and others added 5 commits August 1, 2018 17:04

fix(core): throw error message when @output not initialized

35f0135

Closes angular#3664

fix(core): fix tests for uninitialized @output error

7f89fde

fixup! fix(core): fix tests for uninitialized @output error

3a3f400

fixup! fix(core): fix tests for uninitialized @output error

2d58864

fixup! fix(core): throw error message when @output not initialized

375449e

IgorMinar force-pushed the fix/uninitialized-output-error branch from 65dc6b6 to 375449e Compare August 2, 2018 15:27

IgorMinar added cla: yes and removed cla: no labels Aug 2, 2018

IgorMinar approved these changes Aug 2, 2018

View reviewed changes

IgorMinar closed this in adf510f Aug 2, 2018

IgorMinar pushed a commit that referenced this pull request Aug 2, 2018

fix(core): fix tests for uninitialized @output error (#19116)

183757d

PR Close #19116

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019

	export function isObservable(obj: any \| Observable<any>): obj is Observable<any> {
	// TODO: use Symbol.observable when https://github.com/ReactiveX/rxjs/issues/2415 will be resolved
	return !!obj && typeof obj.subscribe === 'function';
	}

Conversation

aldo-roman commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

googlebot commented Sep 8, 2017

Uh oh!

aldo-roman commented Sep 8, 2017

Uh oh!

googlebot commented Sep 8, 2017

Uh oh!

aldo-roman commented Sep 8, 2017

Uh oh!

googlebot commented Sep 8, 2017

Uh oh!

cedvdb commented Sep 10, 2017

Uh oh!

ngbot Bot commented Jan 16, 2018

Uh oh!

splincode commented Jul 26, 2018

Uh oh!

ngbot Bot commented Jul 27, 2018

Uh oh!

googlebot commented Jul 27, 2018

Uh oh!

googlebot commented Jul 27, 2018

Uh oh!

mhevery commented Jul 27, 2018

Uh oh!

vicb Jul 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

splincode Jul 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IgorMinar Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IgorMinar Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

splincode Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

vicb Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trotyl Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

splincode Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trotyl Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

splincode Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Aug 1, 2018

Uh oh!

googlebot commented Aug 1, 2018

Uh oh!

googlebot commented Aug 2, 2018

Uh oh!

googlebot commented Aug 2, 2018

Uh oh!

angular-automatic-lock-bot Bot commented Sep 13, 2019

aldo-roman commented Sep 8, 2017 •

edited

Loading

vicb Jul 28, 2018 •

edited

Loading

splincode Jul 28, 2018 •

edited

Loading

IgorMinar Jul 30, 2018 •

edited

Loading

vicb Jul 31, 2018 •

edited

Loading

trotyl Jul 31, 2018 •

edited

Loading

splincode Jul 31, 2018 •

edited

Loading

splincode Jul 31, 2018 •

edited

Loading