Module Augmentation #1388

david-driscoll · 2016-02-24T23:06:10Z

This combines #1284 and #1288 with the latest additions to the signatures, and introduces "module augmentation" for use with Rx.

Thanks to @masaeedu for getting it going, this branch is just an extension of his work. 👏

In addition I was able to clean up Rx.DOM.ts and Rx.KitchenSink.ts, so that they simply export Rx.ts, plus add the additional augmented items.

cc @kwonoj @Blesh @staltz @luisgabriel

kwonoj · 2016-02-24T23:07:32Z

#1284 and #1288 can be closed on behalf of this? (actually, noticed one of them closed already)

kwonoj · 2016-02-24T23:09:38Z

src/Subject.ts

@@ -15,8 +15,9 @@ export class Subject<T> extends Observable<T> implements Observer<T>, Subscripti
    return new Subject<T>(destination, source);
  };

-  constructor(protected destination?: Observer<T>, protected source?: Observable<T>) {
+  constructor(protected destination?: Observer<T>, source?: Observable<T>) {


exposing source is expected changes in this PR?

This doesn't actually change the visibility of source, since it is already a protected property. You're right though, in that it doesn't really seem like it belongs in this PR. It's probably something that crept in while I was fooling around with #1234. @david-driscoll will probably have to pull this out 🙏

since it is already a protected property.

: Yes, you're correct. I've missed inheritance.

Let's take this out. :)

No that was unintentional (I was seeing a bug in my editor that was already fixed)

masaeedu · 2016-02-24T23:09:53Z

@kwonoj Yeah, this PR amalgamates the changes in both of those, alongside some changes that were only mentioned in comments (cleanup of Kitchensink to re-export from Rx, for example). Correct me if I'm wrong here @david-driscoll.

kwonoj · 2016-02-24T23:14:51Z

Main question in here will be 'when' to check in this changes. I think it's ok to consider now since 1.8 is released, ng2 b7 is 1.8 friendly, so next release of RxJS will work with those mostly. cc @robwormald and @IgorMinar for visibility, to not proceed if that's incorrect expectation.

Also, maybe we need commit message for 'breaking changes'? (not sure, this is non-api changes but prerequisite dependency changes)

kwonoj · 2016-02-24T23:16:53Z

src/Observable.ts

@@ -127,7 +20,7 @@ export type ObservableInput<T> = ObservableOrPromise<T> | ArrayOrIterator<T>;
 *
 * @class Observable<T>
 */
-export class Observable<T> implements CoreOperators<T>  {
+export class Observable<T> {


so interface coreoperators and kitchensinkoperators are gone completely?

They are, I don't see a real need for them, though if we needed too we could bring them back I suppose. After all the file was, was a list of signatures and to an interface, which is what we've replaced with this PR.

I don't see a real need for them

: In general I have similar feeling since I didn't have lot of cases of use interface to observable instead of observable itself but might be some usecases I couldn't think of. cc @Blesh for opinions.

luisgabriel · 2016-02-24T23:17:41Z

Amazing work guys! 👏 👏

kwonoj · 2016-02-24T23:18:49Z

Amazing work guys!

: Yes, I forgot to add this emoji... 👏 👍

kwonoj · 2016-02-25T01:01:07Z

Seems Rx.TestScheduler is not exported in kitchensink?

david-driscoll · 2016-02-25T15:54:21Z

@kwonoj yeah... I was missing TimeInterval, TestScheduler and VirtualTimeScheduler from the original file. Those have been added back in.

masaeedu · 2016-02-25T16:58:12Z

src/Rx.DOM.ts

-import './add/observable/throw';
-import './add/observable/timer';
-import './add/observable/zip';
+export * from './Rx';


Not 100% sure, but I think this might need to be import './Rx' now, since the augmentations are treated as a side effect. They recently changed what gets emitted in declaration files from imports/exports; see microsoft/TypeScript#6835

EDIT: Same for Rx.KitchenSink

I'll look into it.

@masaeedu you saying we need to import and export it? If I just change it to import things break. If I only export, I get correct typings my editor, and everything seems to be working fine.

@david-driscoll Actually, I just realized this is a non-issue, since Rx exports lots of stuff. The issue was that if you try to export * from 'foo', and foo has important module augmentations but nothing exported, the export * from will be elided from the definition file, and you lose the augmentations.

gotcha, so that means 👍 to me not being crazy! 😄

robwormald · 2016-02-29T04:25:03Z

great stuff! 👏

cc @IgorMinar

benlesh · 2016-03-01T00:58:47Z

Very nice @david-driscoll!

benlesh · 2016-03-01T00:59:15Z

@kwonoj, can I lean on you to verify this change?

kwonoj · 2016-03-01T01:02:34Z

@Blesh Sure. I already reviewed most of changes and mostly fine, was waiting good to go sign from you (and from ng2 also) - I'll respin verification and ping once it's ready to check in.

robwormald · 2016-03-01T01:03:07Z

will verify on our end tomorrow

kwonoj · 2016-03-01T01:06:57Z

@robwormald cool, I'll finish verification on my end before then, can be aligned.

kwonoj · 2016-03-01T02:38:32Z

Change looks good to me.

@Blesh , one thing to be noted - as mentioned in here (#1388 (comment)) , now CoreOperators and KitchenSinkOperators interfaces are removed by this PR. I don't see strong usecases of those interface since most of behaviors are based on Observable<T> itself so think it's ok but letting you know since there may be some reason it should exist.

kwonoj · 2016-03-07T23:59:09Z

Remaining things

rebase : @david-driscoll
good to go sign from ng 2 for align release schedule : @robwormald

benlesh · 2016-03-08T01:17:26Z

good to go sign from ng 2 for align release schedule

What's that?

Also: @kwonoj I've added a label for "needs rebase".

kwonoj · 2016-03-08T01:39:39Z

@Blesh nothing special - basically this change will make minimum requirement of TypeScript to 1.8.2. Check once again Angular 2 also bumps up minimum requirement before check in to avoid breaking build for further releases. I believe this won't happen but just in case to ensure.

Use new external module augmentation feature to move Observable stubs into patch files. See ReactiveX#1193

… patch files

david-driscoll · 2016-03-09T15:29:46Z

@kwonoj @robwormald rebased

kwonoj · 2016-03-09T20:29:48Z

Thanks, I'll check this in around later today.

kwonoj · 2016-03-10T06:14:08Z

Merged with a08fabf, thanks @david-driscoll ! 🎉

lock · 2018-06-07T08:02:50Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

masaeedu mentioned this pull request Feb 24, 2016

WAITING FOR TSC 1.8: refactor(patches): move operator stubs from Observable.ts to patch files #1284

Closed

kwonoj reviewed Feb 24, 2016
View reviewed changes

kwonoj mentioned this pull request Feb 24, 2016

WAITING FOR TSC 1.8: Move static observable stubs from Observable.ts to… #1288

Closed

kwonoj reviewed Feb 24, 2016
View reviewed changes

david-driscoll force-pushed the augmentation branch 2 times, most recently from 3cac7f7 to e037660 Compare February 24, 2016 23:51

david-driscoll force-pushed the augmentation branch from e037660 to a979cdf Compare February 25, 2016 15:46

masaeedu reviewed Feb 25, 2016
View reviewed changes

kwonoj added the PR: lgtm label Mar 2, 2016

benlesh added the PR: needs rebase label Mar 8, 2016

kwonoj removed the PR: lgtm label Mar 8, 2016

masaeedu and others added 2 commits March 9, 2016 10:16

refactor(patches): move operator stubs from Observable.ts to patch files

a5904e5

Use new external module augmentation feature to move Observable stubs into patch files. See ReactiveX#1193

refactor(build): standardized how Rx.KitchenSink and Rx.DOM are built

9dbccf4

refactor(patches): move static observable stubs from Observable.ts to…

a08fabf

… patch files

david-driscoll force-pushed the augmentation branch from a979cdf to a08fabf Compare March 9, 2016 15:23

kwonoj added PR: lgtm and removed PR: needs rebase labels Mar 9, 2016

kwonoj merged commit a08fabf into ReactiveX:master Mar 10, 2016

kwonoj mentioned this pull request Mar 10, 2016

Cannot opt-in import operator's Type Definitions from 'rxjs/add/operator/***' #1193

Closed

kwonoj mentioned this pull request Mar 21, 2016

Type definition for extended operator is not published #642

Closed

lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Module Augmentation #1388

Module Augmentation #1388

david-driscoll commented Feb 24, 2016

kwonoj commented Feb 24, 2016

kwonoj Feb 24, 2016

masaeedu Feb 24, 2016

kwonoj Feb 24, 2016

david-driscoll Feb 24, 2016

masaeedu commented Feb 24, 2016

kwonoj commented Feb 24, 2016

kwonoj Feb 24, 2016

david-driscoll Feb 24, 2016

kwonoj Feb 25, 2016

luisgabriel commented Feb 24, 2016

kwonoj commented Feb 24, 2016

kwonoj commented Feb 25, 2016

david-driscoll commented Feb 25, 2016

masaeedu Feb 25, 2016

david-driscoll Feb 26, 2016

david-driscoll Feb 26, 2016

masaeedu Feb 26, 2016

david-driscoll Feb 26, 2016

robwormald commented Feb 29, 2016

benlesh commented Mar 1, 2016

benlesh commented Mar 1, 2016

kwonoj commented Mar 1, 2016

robwormald commented Mar 1, 2016

kwonoj commented Mar 1, 2016

kwonoj commented Mar 1, 2016

kwonoj commented Mar 7, 2016

benlesh commented Mar 8, 2016

kwonoj commented Mar 8, 2016

david-driscoll commented Mar 9, 2016

kwonoj commented Mar 9, 2016

kwonoj commented Mar 10, 2016

lock bot commented Jun 7, 2018

Module Augmentation #1388

Module Augmentation #1388

Conversation

david-driscoll commented Feb 24, 2016

kwonoj commented Feb 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masaeedu commented Feb 24, 2016

kwonoj commented Feb 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisgabriel commented Feb 24, 2016

kwonoj commented Feb 24, 2016

kwonoj commented Feb 25, 2016

david-driscoll commented Feb 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robwormald commented Feb 29, 2016

benlesh commented Mar 1, 2016

benlesh commented Mar 1, 2016

kwonoj commented Mar 1, 2016

robwormald commented Mar 1, 2016

kwonoj commented Mar 1, 2016

kwonoj commented Mar 1, 2016

kwonoj commented Mar 7, 2016

benlesh commented Mar 8, 2016

kwonoj commented Mar 8, 2016

david-driscoll commented Mar 9, 2016

kwonoj commented Mar 9, 2016

kwonoj commented Mar 10, 2016

lock bot commented Jun 7, 2018