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

david-driscoll · 2016-02-02T20:26:03Z

… patch files

Related to #1284, and I'm fine with rebasing once #1284 gets merged.

kwonoj · 2016-02-02T20:28:08Z

👍 Thanks for PR, let's take #1284 first, hope you do not mind. (I know you already mentioned it's ok ;) )

kwonoj · 2016-02-02T20:29:17Z

src/add/observable/bindNodeCallback.ts

+declare module '../../Observable' {
+  module Observable {
+    export let bindNodeCallback: typeof BoundNodeCallbackObservable.create;


Just out of curiosity - can let be const?

It appears the compiler will then refuse to let you assign the value above.

hmm.. interesting, thanks. Not a major issue, so think it'd be ok.

kwonoj · 2016-02-02T20:48:17Z

cc @Blesh also for visibility, @Blesh , this PR changes organizations of static method as below

now Observable.ts does not include static method stubs by default
as same as patch operator, importing patch will dynamically import static method as well as its types
No changes for Rx / Rx.KitchenSink consumer perspective. Internally now it imports patch operator then reexports to type definitions are published.

david-driscoll · 2016-02-02T20:58:51Z

One thing to note, today they still have to import the static method, but a TypeScript consumer will see that the method "exists" then when they try to use it they'll get a method not found error.

This change makes it obvious that method doesn't exist and that they need to import it.

kwonoj · 2016-02-02T21:00:17Z

Yes, my terms are not precise. @david-driscoll 's correct. removed incorrect comment.

benlesh · 2016-02-02T21:25:54Z

Will this all work correctly if the consumer is using an older version of TypeScript? (say 1.7 or 1.8?)

david-driscoll · 2016-02-02T21:28:42Z

I believe some of the changes are being back ported to 1.8 but I would have to confirm that. This would also affect #1284. @masaeedu do you know if this is being back ported to 1.8? I thought I saw something about that.

kwonoj · 2016-02-02T21:31:13Z

I believe it should work at least on 1.8, for earlier version need to verify it. Primary goal is not to regress and consumer should not care about typescript version dependency. (this, #1284 both)

david-driscoll · 2016-02-02T21:33:10Z

microsoft/TypeScript#6731 is tagged as version 1.8.2

kwonoj · 2016-02-02T21:42:13Z

Just tried and verified this change won't work with previous version of typescript, i.e 1.7. So if typescript side fix are in 1.8.2, supported min version will be 1.8.2. This feels bit breaking - will there be user want to stay with 1.6, or 1.7?

masaeedu · 2016-02-02T22:01:26Z

This change won't work with TypeScript 1.7.5, no. You need to have a version of the compiler with the external augmentations feature enabled in order for #1284 and this PR to work, so you need to have either the nightlies or 1.8 (which is indeed being back ported to).

masaeedu · 2016-02-02T22:03:55Z

My personal opinion is that there aren't any very serious reasons for a developer to want to stick to TS 1.7, since the breaking changes between 1.7-1.8 are so few, and the new features so many. That said I appreciate that breaking changes should be taken seriously, and will leave it to the maintainers to decide how to proceed.

kwonoj · 2016-02-02T22:08:39Z

I personally living edge of versions so wouldn't mind to upgrade and would vote for this changes, but due to that reason I'm not able to represent actual consumer perspective. @Blesh , what do you think? If version support is concern, I think we can hold off these changes for now since this is not blocking any functional scope of current codebase.

benlesh · 2016-02-02T22:22:16Z

TypeScript is currently published at version 1.7 and 1.8 is beta. So we need to support 1.7, unfortunately.

kwonoj · 2016-02-02T22:25:25Z

published at version 1.7 and 1.8 is beta. So we need to support 1.7

: That seems quite legit, valid point. @masaeedu / @david-driscoll , in that case I'd like to suggest to hold this changes until TypeScript release bumps up to 1.8 to avoid versioning issues - any suggestions around?

benlesh · 2016-02-02T23:35:35Z

Let's have the Angular folks chime in on this too, since this is their big ask: @IgorMinar @jeffbcross @vsavkin @robwormald

robwormald · 2016-02-02T23:50:16Z

definitely 👍 on this PR in general, but we do need to hold until this functionality is released properly. we can't ask users to install a specific TS edge version (not to mention then having to override their editor to use it as well...)

kwonoj · 2016-02-03T00:22:50Z

I agree it's hard to recommend consumers to tie with bleeding edge version of infrastructure.

Once I got agreement from @david-driscoll and @masaeedu , I'll put on hold both of PR and re raise once TypeScript bumps up its stable version includes this support. Hope it comes soon, I also quite like this changes and it possibly can bring additional benefit to codebases.

david-driscoll · 2016-02-03T01:37:43Z

Oh it's fine to hold it. We can't all ride on the bleeding edge. 👍

masaeedu · 2016-02-03T18:27:05Z

@kwonoj No objection here, waiting for 1.8 makes sense.

masaeedu · 2016-02-04T04:04:01Z

Note for when we get back to this: microsoft/TypeScript#6858 fixes module imports so that re-exports aren't needed. TL;DR, all those import 'foo' -> import 'foo'; export * from 'foo'; changes are no longer required.

kwonoj · 2016-02-04T04:04:52Z

Great news :)

mhegazy · 2016-02-04T19:51:56Z

src/add/observable/bindCallback.ts

+declare module '../../Observable' {
+  module Observable {


why not namespace instead of module?

Reasoning: the key word module in these two lines mean two things, and it can be confusing. namespace would be clearer.

mainly I'm just used to using module for so long. I have no issues changing it to namespace.

Yes please, if that's not too much trouble. The less overloaded terms I need to disambiguate in my head the better 😄

masaeedu · 2016-02-04T20:07:51Z

@david-driscoll Since this only works in bleeding edge TypeScript anyways, I think you can also take advantage of microsoft/TypeScript#6858 and get rid of the export * froms. Imports are fixed so those are no longer needed.

david-driscoll · 2016-02-04T20:12:03Z

@masaeedu did you pull them out in your branch? I can do that no problem.

kwonoj · 2016-02-04T20:12:08Z

@masaeedu Is that feature has planned tagged version? I.e 1.8.2 has module aug and if those comes later (1.9 or something else), it might need to be separated.

… patch files

masaeedu · 2016-02-04T20:21:01Z

@kwonoj Yup, it'll be in 1.8.

david-driscoll · 2016-02-04T20:21:05Z

There are a bunch of changes in https://github.com/Microsoft/TypeScript/commits/release-1.8 (the release tracking branch that they are migrating changes into).

do not error if typings for external library is empty .d.ts file microsoft/TypeScript#6846
validation of protected properties is broken when class is merged with interface from another file microsoft/TypeScript#6762
Ports #6858 into release-1.8 microsoft/TypeScript#6906

masaeedu · 2016-02-04T20:22:55Z

@david-driscoll Haven't had time, will get to it on the weekend. No rush, right? 😢

david-driscoll · 2016-02-04T20:24:17Z

@masaeedu just want to make sure we're lined up! 😄

Now just watch them release 1.8 tomorrow. 😱

IgorMinar · 2016-02-05T01:31:38Z

This looks good to me but we need to coordinate merging this with a TS release and an Angular release. We can't ask people to be on unstable release of typescript.

kwonoj · 2016-02-22T22:39:18Z

TS 1.8 has released (https://github.com/Microsoft/TypeScript/releases/tag/v1.8.2). Might need align with ng2 still though.

robwormald · 2016-02-24T08:29:11Z

as of ng2beta7 we're 1.8 friendly I believe.

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

as of ng2beta7 we're 1.8 friendly I believe.

: Lovely ❤️

kwonoj · 2016-02-24T23:11:22Z

@david-driscoll , I'm closing this PR per #1388. Let me know if this is incorrect.

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

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.

kwonoj reviewed Feb 2, 2016
View reviewed changes

kwonoj added the type: discussion label Feb 2, 2016

kwonoj mentioned this pull request Feb 2, 2016

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

Closed

kwonoj added blocked and removed type: discussion labels Feb 3, 2016

benlesh changed the title ~~refactor(patches): move static observable stubs from Observable.ts to…~~ WAITING FOR TSC 1.8: Move static observable stubs from Observable.ts to… Feb 3, 2016

mhegazy reviewed Feb 4, 2016
View reviewed changes

david-driscoll force-pushed the static-typing-patches branch from 04a6085 to 6c6bf10 Compare February 4, 2016 20:00

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

2e6da38

… patch files

david-driscoll force-pushed the static-typing-patches branch from 6c6bf10 to 2e6da38 Compare February 4, 2016 20:14

david-driscoll mentioned this pull request Feb 24, 2016

Module Augmentation #1388

Merged

kwonoj closed this Feb 24, 2016

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

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

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

Conversation

david-driscoll commented Feb 2, 2016

kwonoj commented Feb 2, 2016

kwonoj Feb 2, 2016

Choose a reason for hiding this comment

david-driscoll Feb 2, 2016

Choose a reason for hiding this comment

kwonoj Feb 2, 2016

Choose a reason for hiding this comment

kwonoj commented Feb 2, 2016

david-driscoll commented Feb 2, 2016

kwonoj commented Feb 2, 2016

benlesh 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

masaeedu commented Feb 2, 2016

masaeedu commented Feb 2, 2016

kwonoj commented Feb 2, 2016

benlesh commented Feb 2, 2016

kwonoj commented Feb 2, 2016

benlesh commented Feb 2, 2016

robwormald commented Feb 2, 2016

kwonoj commented Feb 3, 2016

david-driscoll commented Feb 3, 2016

masaeedu commented Feb 3, 2016

masaeedu commented Feb 4, 2016

kwonoj commented Feb 4, 2016

mhegazy Feb 4, 2016

Choose a reason for hiding this comment

david-driscoll Feb 4, 2016

Choose a reason for hiding this comment

masaeedu Feb 4, 2016

Choose a reason for hiding this comment

masaeedu commented Feb 4, 2016

david-driscoll commented Feb 4, 2016

kwonoj commented Feb 4, 2016

masaeedu commented Feb 4, 2016

david-driscoll commented Feb 4, 2016

masaeedu commented Feb 4, 2016

david-driscoll commented Feb 4, 2016

IgorMinar commented Feb 5, 2016

kwonoj commented Feb 22, 2016

robwormald commented Feb 24, 2016

kwonoj commented Feb 24, 2016

kwonoj commented Feb 24, 2016

lock bot commented Jun 7, 2018