Skip to content

chore(async): Refactor EventEmitter and Async Facade#4893

Closed
robwormald wants to merge 4 commits into
angular:masterfrom
robwormald:eventemit
Closed

chore(async): Refactor EventEmitter and Async Facade#4893
robwormald wants to merge 4 commits into
angular:masterfrom
robwormald:eventemit

Conversation

@robwormald
Copy link
Copy Markdown
Contributor

Refactor EventEmitter and Async Facade to match ES7 Observable semantics, properly use RxJS typedefs, make EventEmitter inherit from RxJS Subject. Closes #4149.

BREAKING CHANGE:

  • consumers of EventEmitter no longer need to call .toRx()
  • EventEmitter is now generic and requires a type - e.g. EventEmitter<string>
  • EventEmitter and Observable now use the .subscribe(generatorOrNext, error, complete) method instead of .observer(generator)
  • ObservableWrapper uses callNext/callError/callComplete instead of callNext/callThrow/callReturn

Notes:

  • This uses the RxJS main import path (though not the "kitchen sink"), and as such adds a number of methods to the public API (see public_api_spec). Note that Subject and EventEmitter both inherit from Observable and so it's less drastic than it looks in terms of new operators ;-) This is the current best workaround / compromise between having accurate type information and a usable EventEmitter / Observable interface. Reference discussions in Determine which Observable operators and extensions to include in angular core #4390, and over in Rx Move core operators onto Observable directly ReactiveX/rxjs#468 for a potential middle-term solution. This does add bytes to bundles, though with the incoming changes to bundling this could mostly be mitigated. In my opinion, this gives a better developer experience and we can watch over the beta period how and which operators should define the core.
  • Currently this fails on CI due to the dgeni generation of types not liking Rx's external .d.ts files - see Remove deprecated dgeni dgeneration of d.ts files from gulp and CI #4967 but passes otherwise, and I'll rebase once that clears.
  • It adds a psuedo Subject to the dart async facade that matches the EventEmitter - this could potentially be resolved by changes in ts2dart @mprobst ?
  • tldr: this fixes a number of annoying issues re: developer experience (types and operators with EventEmitter, removes .toRx(), etc) and I think is a reasonable step to land before beta. I have a number of minor fixes (Http typedefs, etc) to follow to further improve said experience.

@robwormald robwormald force-pushed the eventemit branch 8 times, most recently from 240dfc3 to 89a3fc8 Compare October 26, 2015 23:20
@robwormald robwormald force-pushed the eventemit branch 4 times, most recently from 2a2b667 to 671721e Compare October 28, 2015 02:46
@robwormald robwormald changed the title chore(async): update Rx import path chore(async): Refactor EventEmitter and Async Facade Oct 28, 2015
@robwormald
Copy link
Copy Markdown
Contributor Author

cc @jeffbcross per our discussions
cc @Blesh per the "shipping Observable without operators is like shipping a keg of beer without a tap" commentary
cc @vsavkin to approve public API changeset.

@naomiblack naomiblack added this to the beta-00 milestone Oct 28, 2015
@naomiblack naomiblack added breaking changes action: review The PR is still awaiting reviews from at least one requested reviewer and removed pr_state: LGTM labels Oct 28, 2015
@naomiblack
Copy link
Copy Markdown
Contributor

@vsavkin -- this previously had an LGTM from @mhevery but needs another review pass since the code has changed. Can you have a look?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: maybe move closer to the import?

@mprobst
Copy link
Copy Markdown
Contributor

mprobst commented Oct 28, 2015

I think the specific Subject<T> class in async.dart is a perfectly fine solution for this (AFAICT). Unless it causes any issues, I'd just go with this.

@benlesh
Copy link
Copy Markdown
Contributor

benlesh commented Oct 28, 2015

Dart just needs an Rx library of it's own, IMO. You could even transpile the TypeScript from RxJS 5 over directly.

@vicb
Copy link
Copy Markdown
Contributor

vicb commented Oct 28, 2015

Dart just needs an Rx library of it's own, IMO. You could even transpile the TypeScript from RxJS 5 over directly.

May be https://github.com/frankpepermans/rxdart can help short term ?

@benlesh
Copy link
Copy Markdown
Contributor

benlesh commented Oct 28, 2015

@vicb that library looks like it's wrapping an older version of RxJS and exposing it in Dart.

From my conversations with @IgorMinar, I think that the Angular team has the means to transpile TypeScript to Dart directly. Given that RxJS 5 is written entirely in TypeScript, and really isn't using any exotic JavaScript types, I think that it could be transpiled directly to Dart as well.

@robwormald robwormald force-pushed the eventemit branch 2 times, most recently from 6c58cb2 to d27d7af Compare October 29, 2015 00:18
@mhevery mhevery self-assigned this Oct 29, 2015
@robwormald robwormald force-pushed the eventemit branch 2 times, most recently from 41916e1 to 3a8b8ee Compare October 29, 2015 05:01
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Oct 29, 2015

@robwormald, can we get this green and merged.

@mhevery mhevery added pr_state: LGTM and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 29, 2015
@jeffbcross jeffbcross assigned robwormald and unassigned mhevery Oct 29, 2015
Refactor EventEmitter and Async Facade to match ES7 Observable semantics, properly use RxJS typedefs, make EventEmitter inherit from RxJS Subject. Closes angular#4149.

BREAKING CHANGE:
- consumers of EventEmitter no longer need to call .toRx()
- EventEmitter is now generic and requires a type - e.g. `EventEmitter<string>`
- EventEmitter and Observable now use the `.subscribe(generatorOrNext, error, complete)` method instead of `.observer(generator)`
- ObservableWrapper uses `callNext/callError/callComplete` instead of `callNext/callThrow/callReturn`
Makes ObservableWrapper and AsyncPipe work with Observable, Subject, and EventEmitter
@jeffbcross jeffbcross added action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Oct 29, 2015
r-park added a commit to r-park/todo-angular-firebase that referenced this pull request Nov 13, 2015
- EventEmitter is now generic and requires a type [angular/angular#4893]
- EventEmitter and Observable now use the .subscribe(generatorOrNext, error, complete) method instead of .observer(generator) [angular/angular/pull/4893]
- tsconfig: set `moduleResolution: node` - angular/angular#5248
- imports: switch to file-relative paths
r-park added a commit to r-park/todo-angular-firebase that referenced this pull request Nov 13, 2015
- npm: update dependencies
- AuthService, TaskStore: EventEmitter is now generic and requires a type [angular/angular#4893]
- AuthService, TaskStore: EventEmitter and Observable now use the .subscribe(generatorOrNext, error, complete) method instead of .observer(generator) [angular/angular/pull/4893]
- tsconfig: set `moduleResolution: node` [angular/angular#5248]
- imports: switch to file-relative paths
- docs: update readme
sbvhev pushed a commit to sbvhev/todo-angular-firebase that referenced this pull request May 29, 2018
- npm: update dependencies
- AuthService, TaskStore: EventEmitter is now generic and requires a type [angular/angular#4893]
- AuthService, TaskStore: EventEmitter and Observable now use the .subscribe(generatorOrNext, error, complete) method instead of .observer(generator) [angular/angular/pull/4893]
- tsconfig: set `moduleResolution: node` [angular/angular#5248]
- imports: switch to file-relative paths
- docs: update readme
@angular-automatic-lock-bot
Copy link
Copy Markdown

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 7, 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 breaking changes cla: yes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(facade): change EventEmitter to match RxJS-Next Subject (and rename plz)

8 participants