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

masaeedu · 2016-02-02T07:27:12Z

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

kwonoj · 2016-02-02T08:12:43Z

Appreciate for taking effort for this!

I tried this PR and found somewhat unexpected behavior, while consumption of augmented module works editor supports for typing isn't as below,

none of type definitions are now being supported by editors.

This change solves build time detection of what issue #1193 points but this seems quite regression that prevents daily usage of type definitions. I assume current editor's TypeScript engines are yet able to support those.

kwonoj · 2016-02-02T08:14:27Z

For checking in this changes, I came to bit hesitate by this change loses existing availability of typings - What do you think? Is there'll be immediate resolution for this? /cc @saneyuki, @Blesh also for visibility.

masaeedu · 2016-02-02T08:22:16Z

@kwonoj Are you importing the appropriate operators? Also, could you provide a testcase I can stick in a file and play around with?

kwonoj · 2016-02-02T08:23:49Z

@masaeedu That's occurring even importing full set, import * as Rx from... Since full set imports every operator, I'd expect it work out of box as same as before.

kwonoj · 2016-02-02T08:24:31Z

Just clarify, I'm currently using VS code. Curious if other editor / IDE might have same behavior.

masaeedu · 2016-02-02T08:33:46Z

@kwonoj Weird. Trying to setup a test now. Just to be sure, have you set your "typescript.tsdk" setting at the user or workspace level to the latest typescript?

kwonoj · 2016-02-02T08:35:26Z

have you set your "typescript.tsdk" setting at the user or workspace level to the latest typescript?

: No, I'm using out of box configuration. Maybe that's reason?

masaeedu · 2016-02-02T08:48:04Z

@kwonoj Ok, so that's probably one thing to fix. I got some great help with how to do this in microsoft/vscode#1771. However, I already have this set up and can reproduce the issue you're seeing when I import * as Rx from 'rxjs/Rx' (but not when importing individual patch files).

The issue is that because we're not re-exporting the contents of the patch files in the Rx.ts file, all the import './add/operator/foo' lines have no effect on the generated .d.ts file. What we want is to do export * from './add/operator/foo'. I've tried changing a few of the imports to re-exports and it seems to work.

kwonoj · 2016-02-02T08:51:15Z

What we want is to do export * from './add/operator/foo'

: What happens if import operators separately?

masaeedu · 2016-02-02T08:52:08Z

@kwonoj You mean if I don't use the rxjs/Rx module and import directly from add/operator/foo? That scenario works fine for me right now. Only the operators I import are available on an Observable.

kwonoj · 2016-02-02T08:53:27Z

if I don't use the rxjs/Rx module and import directly from add/operator/foo?

: Yes, that was the case.

kwonoj · 2016-02-02T08:54:33Z

When I tried both (importing Rx / importing operator individually) were not able to provide type definitions.

masaeedu · 2016-02-02T09:00:57Z

So this is the scenario I'm trying right now:

// @filename:main.ts
import {Observable} from 'rxjs/Observable';
import 'rxjs/add/operator/buffer';

var obs = Observable.of(1, 2, 3);
obs.bu // I press Ctrl+Space to get autocomplete options here

The autocomplete options I get look like this:

(please ignore tslint squigglies 😄). Once I change the corresponding imports in Rx.ts to export * froms, they start working the same way.

masaeedu · 2016-02-02T09:04:20Z

@kwonoj Hopefully this is what the expected behavior is?

kwonoj · 2016-02-02T09:04:32Z

Cool, it's lovely. We still need to test out couple of more editor / IDE to ensure this works though. (anyone uses other editor than VS code?)

kwonoj · 2016-02-02T09:07:07Z

We still need to test out couple of more editor / IDE to ensure this works though.

: I think it'd be ok as long as other editor's not regressed. losing all type intellisense is case I'd like to avoid.

masaeedu · 2016-02-02T09:13:44Z

@kwonoj I can test out Atom. There's no rush to get this merged, I don't think rebasing this should be too difficult because there's not too many people fooling around with the patch files.

masaeedu · 2016-02-02T09:15:08Z

@kwonoj Also, now that I've fixed the Rx.ts file, the scenario you were talking about works 😄 :

kwonoj · 2016-02-02T09:15:56Z

Thanks, let's test out couple of more and rebase. Might need see how does reexport looks like.

kwonoj · 2016-02-02T09:16:32Z

the scenario you were talking about works

: Cool, one step forward for better typing experiences :)

masaeedu · 2016-02-02T09:17:58Z

I did however have to nuke the export var _void: void; in the patch files to make the re-exports work. Do you know if this is still needed?

kwonoj · 2016-02-02T09:20:09Z

this is still needed

Nope. That was introduced due to #1010, patch operator type definition becomes empty since there's nothing exported. If there's real export, that doesn't necessarily required anymore.

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

Change all `import './add/operator/foo'` in Rx.ts to `export * from './add/operator/foo'`, in order to ensure the module augmentations appear in the generated Rx.d.ts file. Also remove `export var _void`s to avoid conflicts.

kwonoj · 2016-02-02T09:40:37Z

What about Rx.KitchenSink? doesn't it need to be updated as well?

kwonoj · 2016-02-02T09:42:22Z

src/add/observable/combineLatest.ts

@@ -1,6 +1,4 @@
 import {Observable} from '../../Observable';
 import {combineLatestStatic} from '../../operator/combineLatest';

-Observable.combineLatest = combineLatestStatic;
-
-export var _void: void;


isn't this still needed? does typings contains exports after removing this?

I'm not sure what you mean by that. This file is still considered an external module after removal of export var _void if that's what you mean, because it has top level imports.

Refer #1010 I mentioned. If there isn't any export makes d.ts to be empty, it makes consumer in trouble. So question's is there anything exported after removing this and type definition's published. I assume operator includes augmentation does.

@kwonoj Yes, I ran into that issue. Discussing with the TypeScript folks in microsoft/TypeScript#6022

For our side of interim, we could leave this as same as before, right?

Yes, that's right. I'll revert the _void: void removals and re-exports for the observable patches.

Let's go that way. I don't think this need to block this whole PR. (export void if empty, remove if it's unnecessary)

I'll be reverting these to help the PR go through, but the good news is that empty .d.ts files won't be a problem for the consumer starting in tomorrow's builds (in other words we don't need the _void: void anymore).

That's cool news, but it's starting from tomorrow's nightly version, right? thinking about discussion for min version support at #1288 (comment), that might need to be hold off for moment :(

@kwonoj Yeah, unfortunately it looks like we'll have to let this and #1288 stew for a while. 😞

masaeedu · 2016-02-02T09:45:43Z

@kwonoj I guess so. I wasn't actually aware of the existence of Rx.KitchenSink. Is the goal of that file to export everything in Rx plus some extras? If so it would probably be a good idea to only do export * from './Rx' and then export the extras, instead of duplicating all of them.

luisgabriel · 2016-02-02T13:11:02Z

src/add/operator/buffer.ts

+    buffer: (closingNotifier: Observable<any>) => Observable<T[]>;
+  }
+}


This may be silly but what do you think about augmenting the module before the prototype assignment?

@luisgabriel I don't mind either approach. Is there a reason you prefer that style?

It makes more sense to me to declare an interface first and then implement it. But this is not a big deal. I'm just commenting to see if this makes sense to someone else other than me. ;)

I'd do as @luisgabriel said when I gave attempt. Honestly I think it's minor and does not impact to anything though.

kwonoj · 2016-02-02T16:17:41Z

Is the goal of that file to export everything in Rx plus some extras?

: Yes, it have few additional operators in additions to Rx.

If so it would probably be a good idea to only do export * from './Rx' and then export the extras, instead of duplicating all of them.

: if that works I think that'd better. I tried those while ago but wasn't able to make it.

david-driscoll · 2016-02-02T17:40:13Z

Atom updates fairly regularly against typescript itself, so the definitions do propagate properly it looks like.

david-driscoll · 2016-02-02T18:12:10Z

Does anyone know of a way that this can be applied to static methods as well? Users will still sett Observable.combineLatest even if they haven't imported rxjs/add/observable/combineLatest.

kwonoj · 2016-02-02T18:14:36Z

that this can be applied to static methods as well

: So far I understand, augmentation's instance for class / interface and not applicable for static.

david-driscoll · 2016-02-02T18:16:07Z

I know, just a random item for thought. Not meant as anything to block, just maybe someone knew something. :)

kwonoj · 2016-02-02T18:18:12Z

, just maybe someone knew something

: Yup, sorry for repeating already-known-to-unknown :) Once this changes are checked in, let's track those as separate issue if needed.

benlesh · 2016-02-02T19:06:07Z

Just marking this as blocked and discussion until it's worked out.

Also it needs to be rebased.

masaeedu · 2016-02-02T19:12:20Z

@david-driscoll I think namespace merging might work for the static methods.

masaeedu · 2016-02-02T19:14:17Z

@david-driscoll Yup, works great:

import {Observable} from '../../../Observable';
import {WebSocketSubject} from '../../../observable/dom/WebSocketSubject';

Observable.webSocket = WebSocketSubject.create;

declare module '../../../Observable' {
  namespace Observable {
    export let webSocket: typeof WebSocketSubject.create;
  }
}

Although we should probably fix the typeof WebSocketSubject.create here eventually for the reasons discussed earlier.

kwonoj · 2016-02-02T19:18:00Z

Let's make static method as separate discussion. This PR itself has quite amount of changes.

david-driscoll · 2016-02-02T19:31:06Z

That's fine, @masaeedu you want to make a separate PR or should I? (I don't care)

robwormald · 2016-02-02T23:53:57Z

same as #1288, i'm definitely for this change but it needs to wait until the functionality is in the released TS compiler.

This is not being used anymore.

masaeedu · 2016-02-24T23:06:51Z

Now tracked by #1388

lock · 2018-06-07T09:02:54Z

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 added 3 commits February 2, 2016 04:26

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

7350287

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

refactor(patches): update Rx.ts to re-export all patch files

61841f6

Change all `import './add/operator/foo'` in Rx.ts to `export * from './add/operator/foo'`, in order to ensure the module augmentations appear in the generated Rx.d.ts file. Also remove `export var _void`s to avoid conflicts.

refactor(patches): move stub to patch file for delayWhen

142ebdb

masaeedu force-pushed the addObservableInterfacePatches branch from 1905fba to 142ebdb Compare February 2, 2016 09:36

kwonoj reviewed Feb 2, 2016
View reviewed changes

fix(Rx.ts): import to prevent erasure in JS files

5abaac1

luisgabriel reviewed Feb 2, 2016
View reviewed changes

benlesh added blocked type: discussion labels Feb 2, 2016

david-driscoll mentioned this pull request Feb 2, 2016

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

Closed

luisgabriel referenced this pull request Feb 3, 2016

chore(tools): remove script to generate operator patches

bb308f4

This is not being used anymore.

kwonoj changed the title ~~refactor(patches): move operator stubs from Observable.ts to patch files~~ WAITING FOR TSC 1.8: refactor(patches): move operator stubs from Observable.ts to patch files Feb 3, 2016

david-driscoll mentioned this pull request Feb 24, 2016

Module Augmentation #1388

Merged

masaeedu closed this Feb 24, 2016

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

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

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

Conversation

masaeedu commented Feb 2, 2016

kwonoj commented Feb 2, 2016

kwonoj commented Feb 2, 2016

masaeedu commented Feb 2, 2016

kwonoj commented Feb 2, 2016

kwonoj commented Feb 2, 2016

masaeedu commented Feb 2, 2016

kwonoj commented Feb 2, 2016

masaeedu commented Feb 2, 2016

kwonoj commented Feb 2, 2016

masaeedu commented Feb 2, 2016

kwonoj commented Feb 2, 2016

kwonoj commented Feb 2, 2016

masaeedu commented Feb 2, 2016

masaeedu commented Feb 2, 2016

kwonoj commented Feb 2, 2016

kwonoj commented Feb 2, 2016

masaeedu commented Feb 2, 2016

masaeedu commented Feb 2, 2016

kwonoj commented Feb 2, 2016

kwonoj commented Feb 2, 2016

masaeedu commented Feb 2, 2016

kwonoj commented Feb 2, 2016

kwonoj commented Feb 2, 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

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

masaeedu commented Feb 2, 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

kwonoj commented Feb 2, 2016

david-driscoll commented Feb 2, 2016

david-driscoll commented Feb 2, 2016

kwonoj commented Feb 2, 2016

david-driscoll commented Feb 2, 2016

kwonoj commented Feb 2, 2016

benlesh commented Feb 2, 2016

masaeedu commented Feb 2, 2016

masaeedu commented Feb 2, 2016

kwonoj commented Feb 2, 2016

david-driscoll commented Feb 2, 2016

robwormald commented Feb 2, 2016

masaeedu commented Feb 24, 2016

lock bot commented Jun 7, 2018