Skip to content

refactor: remove custom isObservable()#39643

Closed
petebacondarwin wants to merge 1 commit intoangular:masterfrom
petebacondarwin:isObservable
Closed

refactor: remove custom isObservable()#39643
petebacondarwin wants to merge 1 commit intoangular:masterfrom
petebacondarwin:isObservable

Conversation

@petebacondarwin
Copy link
Contributor

@petebacondarwin petebacondarwin commented Nov 12, 2020

RXJS introduced the isObservable() method in 6.1.0.
So we can now use that instead of our own function.

@petebacondarwin
refactor: remove custom isObservable()
b6829ae 
RXJS introduced the `isObservable()` method in 6.1.0.
So we can now use that instead of our own function.
@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release labels Nov 12, 2020
@google-cla google-cla bot added the cla: yes label Nov 12, 2020
@petebacondarwin petebacondarwin mentioned this pull request Nov 12, 2020
Closed
14 tasks
@petebacondarwin
Copy link
Contributor Author

petebacondarwin commented Nov 12, 2020 •
edited
Loading

Actually we cannot use RXJS's isObservable() because it has the additional requirement that the object contains the lift() method. See its definition.

Our EventEmitter class does not have this lift() function, so it fails the isObservable() check.
Now it might be that our EventEmitter needs to be fixed, or just that RXJS's isObservable() is too strict.
We need to look into it.

But either way, this PR cannot land as it is.

@petebacondarwin
Copy link
Contributor Author

petebacondarwin commented Nov 13, 2020 •
edited
Loading

It might not be available soon but here is the log of the CI builds that failed for this change:

https://app.circleci.com/pipelines/github/angular/angular/24484/workflows/3bb5785d-3d02-4dff-8c90-edfa90dfe816

Failures:
1) View Providers outputs should listen to provider events
  Message:
    Error: @Output emitter not initialized in 'SomeService'.
  Stack:
    error properties: Object({ ngDebugContext: DebugContext_({ view: Object({ def: Object({ factory: null, nodeFlags: 16385, rootNodeFlags: 1, nodeMatchedQueries: 0, flags: 0, nodes: [ Object({ nodeIndex: 0, parent: null, renderParent: null, bindingIndex: 0, outputIndex: 0, checkIndex: 0, flags: 1, childFlags: 16384, directChildFlags: 16384, childMatchedQueries: 0, matchedQueries: Object({  }), matchedQueryIds: 0, references: Object({  }), ngContentIndex: null, childCount: 1, bindings: [  ], bindingFlags: 0, outputs: [  ], element: Object({ ns: '', name: 'span', attrs: [  ], template: null, componentProvider: null, componentView: null, componentRendererType: null, publicProviders: null({ SomeService_207: Object }), allProviders: null({ SomeService_207: Object }), handleEvent: spy on handleEvent }), provider: null, text: null, query: null, ngContent: null }), Object({ nodeIndex: 1, parent: Object({ nodeIndex: 0, parent: null, renderParent: null, bindingIndex: 0, outputIndex: 0, checkIndex: 0, flags: 1, c ...
    Error: @Output emitter not initialized in 'SomeService'.
        at Object.createDirectiveInstance (packages/core/src/view/provider.ts:153:15)
        at createViewNodes (packages/core/src/view/view.ts:314:28)
        at createRootView (packages/core/src/view/view.ts:216:3)
        at callWithDebugContext (packages/core/src/view/services.ts:638:23)
        at Object.debugCreateRootView [as createRootView] (packages/core/src/view/services.ts:119:10)
        at createRootView (packages/core/test/view/helper.ts:35:19)
        at Object.createAndGetRootNodes (packages/core/test/view/helper.ts:72:16)
        at UserContext.<anonymous> (packages/core/test/view/provider_spec.ts:369:35)
        ....

and the AIO size checker failed with:

FAIL: Commit b6829aee3c9d65887d84dc94f14e52e978de5354 uncompressed main-es2015 exceeded expected size by 500 bytes or >1% (expected: 448922, actual: 449482).

@petebacondarwin petebacondarwin mentioned this pull request Nov 13, 2020
Closed
14 tasks
petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Nov 13, 2020
@petebacondarwin
refactor(core): remove isObservable TODO
5708500 
This commit removes the TODO comment that proposed
that we use the built-in RxJS `isObservable()` function.

This is not a viable approach since the built-in function
requires that the `obj` contains additional methods that
our "observable" types (such as `EventEmitter`) do not
necessarily have.

See angular#39643 for more information.
@petebacondarwin petebacondarwin mentioned this pull request Nov 13, 2020
Closed
atscott pushed a commit that referenced this pull request Nov 16, 2020
@petebacondarwin @atscott
refactor(core): remove isObservable TODO (#39669)
b363d96 
This commit removes the TODO comment that proposed
that we use the built-in RxJS `isObservable()` function.

This is not a viable approach since the built-in function
requires that the `obj` contains additional methods that
our "observable" types (such as `EventEmitter`) do not
necessarily have.

See #39643 for more information.

PR Close #39669
atscott pushed a commit that referenced this pull request Nov 16, 2020
@petebacondarwin @atscott
refactor(core): remove isObservable TODO (#39669)
7bcfc0d 
This commit removes the TODO comment that proposed
that we use the built-in RxJS `isObservable()` function.

This is not a viable approach since the built-in function
requires that the `obj` contains additional methods that
our "observable" types (such as `EventEmitter`) do not
necessarily have.

See #39643 for more information.

PR Close #39669
@petebacondarwin petebacondarwin deleted the isObservable branch November 19, 2020 10:16
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Dec 20, 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 Dec 20, 2020
@pullapprove pullapprove bot added area: common Issues related to APIs in the @angular/common package area: core Issues related to the framework runtime area: forms area: router labels Dec 20, 2020
@ngbot ngbot bot modified the milestone: Backlog Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AndrewKushnir AndrewKushnir Awaiting requested review from AndrewKushnir

@atscott atscott Awaiting requested review from atscott

@jessicajaniuk jessicajaniuk Awaiting requested review from jessicajaniuk

Assignees

No one assigned

Labels

action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package area: core Issues related to the framework runtime area: forms area: router cla: yes refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

1 participant

@petebacondarwin

Comments