chore(typings): Enabled noImplictAny #1077

david-driscoll · 2015-12-15T21:46:18Z

With this change compliation time and performance is improved

@kwonoj This was the quick way, there might a few places where the formatting, or in some cases the variable names used in method signatures that have changed. If I need to reverse those let me know and I'll make a pass at it shortly.

kwonoj · 2015-12-15T21:51:12Z

Appreciate for effort :) few copy-paste from original PR

would you able to align commit message as same as others, starts with no-capital
npm script package.json should be updated as well as tsconfig.json

I'll try to come up with possible cases of suppressImplicitAnyIndexErrors in our codebase meanwhile..

david-driscoll · 2015-12-15T21:57:43Z

done! 😄

~~would you able to align commit message as same as others, starts with no-capital~~
~~npm script package.json should be updated as well as tsconfig.json~~

Build is erroring for suppressImplicitAnyIndexErrors in a few places (that I wasn't seeing, which is strange but not super important here).

Do we want to suppress those messages, or define the types so they don't happen?

kwonoj · 2015-12-15T22:12:14Z

Build is erroring for suppressImplicitAnyIndexErrors in a few places

: Yes, those are errors I could see while I work on this, reason of suggestion for enabling suppressImplicitAnyIndexErrors. I don't see lot of values to enforce type for those indexers in our codebases, i.e if (observerOrNext[rxSubscriber]) {, ish[SymbolShim.observable], etcs.. (but would like to hear your opinion also if you think it'd benefit to have type for those)

david-driscoll · 2015-12-15T22:13:57Z

I don't see much value in typing them, there aren't a lot of cases where Rx is using objects as a dictionary or similar (not when there is map, etc).

I will update to suppress those messages then.

luisgabriel · 2015-12-15T22:14:24Z

src/CoreOperators.ts

@@ -35,9 +35,9 @@ export interface CoreOperators<T> {
                projectResult?: (x: T, y: any, ix: number, iy: number) => R,
                concurrent?: number) => Observable<R>;
  flatMapTo?: <R>(observable: Observable<any>, projectResult?: (x: T, y: any, ix: number, iy: number) => R, concurrent?: number) => Observable<R>;
-  groupBy?: <R>(keySelector: (value: T) => string,
+  groupBy?: <TKey, R>(keySelector: (value: T) => string,


As I suggested in #1050, I think it would be better to rename TKey to K here.

It's the C# dev in me coming out. 😈

I don't care about naming semantics, updating the commit.

luisgabriel · 2015-12-15T22:19:09Z

src/Subject.ts

@@ -63,11 +63,11 @@ export class Subject<T> extends Observable<T> implements Observer<T>, Subscripti
    return new SubjectSubscription(this, subscriber);
  }

-  add(subscription?) {
+  add(subscription?: Subscription<T>) {


You may need to update this if #1065 gets merged first.

kwonoj · 2015-12-15T22:25:46Z

src/add/operator/exhaust.ts

is this file has changed?

I think it's line ending related. There are other files with the same change.

Hrm... I'll see if I can figure out to avoid that line ending issue.

Is there a standard with no newlines at the end of files for the project, or was just that how things happened to end up? (Just want to get it right! :) )

I'm not sure, but I think it's a bit messy at the moment.

I don't think we currently have strict rules for those. Personally I clean up endings if I could find it in my changes, but in this case if file hasn't changed it'd better to unstage simply.

I see the editor config, I'll fix that up.

For this issue, I'll bring up small PR to introduce consistency.

I've gone and corrected the line endings that looked pretty obvious, it looks like some files have them and some don't so it's hard to know if it should or shouldn't lol

some don't so it's hard to know if it should or shouldn't lol

: Yes, agreed. As commented above let me amend those. :)

kwonoj · 2015-12-15T22:53:43Z

I'm seeing some of function signatures are changed not related to implicit any, specifically projection function definition accepts to return Promise. Most of current operator implementation as far as I recall it does not checks projected result is promise or observable. I think this is kind of out-of scope changes requires discussion (will we support those behavior or not?) would like to suggest to not to change those signatures in this PR and create issue if it's necessary.

kwonoj · 2015-12-15T22:55:27Z

src/operator/sample.ts

@@ -19,7 +19,7 @@ class SampleSubscriber<T> extends Subscriber<T> {
  private lastValue: T;
  private hasValue: boolean = false;

-  constructor(destination: Subscriber<T>, private notifier: Observable<any>) {
+  constructor(destination: Subscriber<T>, private notifier: any) {


notifier can be other type than observable?

david-driscoll · 2015-12-23T19:08:08Z

I haven't had a chance to check it, but it's possible it's not needed
anymore.
On Wed, Dec 23, 2015 at 2:07 PM Luís Gabriel Lima notifications@github.com
wrote:

@kwonoj https://github.com/kwonoj I think it'd be better if we could
merge this PR before the change @trxcllnt https://github.com/trxcllnt
and @Blesh https://github.com/blesh are discussing on #1099
#1099 to avoid more rebasing
conflicts.

What do you guys think?

—
Reply to this email directly or view it on GitHub
#1077 (comment).

Thanks,
David

benlesh · 2015-12-23T19:16:28Z

Just a note: This PR should probably wait for @trxcllnt's changes (code reviewed in branch and PR incoming shortly) to the Subscription scheme to land. They're big changes, so it may mean a painful refactor of this PR... however, they're important changes to internals, so they take precedence.

kwonoj · 2015-12-23T19:31:12Z

Agreed with @Blesh here. It wasn't intended to make this PR's check in delayed though, now it's overlaps with major refactoring to subscriptions - in this case, I'd like to let those changes land first prior to this. (Also #1097 as well @luisgabriel , current assumption's those PR's nearly orthogonal to subscription changes, but want to be sure).

luisgabriel · 2015-12-23T20:13:09Z

Ok, no problem. I was assuming that it would be less painful to adapt @trxcllnt's changes since merging this one after could also require adding explicit typing on the new code besides solving the conflicts.

trxcllnt · 2015-12-25T03:25:54Z

@david-driscoll the effort that went into this PR is clearly enormous, so thanks for that. Apologies if rebasing from #1106 is a major headache. I'll do the legwork to reconcile with this PR if necessary.

kwonoj · 2015-12-30T01:10:37Z

#1106 's now landed, @david-driscoll would you able to rebase this? I'll take this as priority to check in once it's rebased, given size of changes delaying it more would create more change conflicts between PRs. (Just note : @luisgabriel , I'll take #1097 as second order after this one. Let me know if you have any suggestions).

luisgabriel · 2015-12-30T01:33:05Z

@kwonoj Ok, no problem from my side. I'll try to rebase this patch tomorrow to see if I can help @david-driscoll to update this PR.

luisgabriel · 2015-12-30T21:40:47Z

@david-driscoll I've rebased your patch on the current master. You can find it in this branch: https://github.com/luisgabriel/RxJS/tree/pr-1077

It was less painful than I thought. ;)

benlesh · 2016-01-04T20:09:50Z

The sooner we get the rebased version in the PR (or PR'ed elsewhere) the sooner we can review the changes again after the rebase and merge this.

benlesh · 2016-01-04T20:10:20Z

Marking "Blocked" as it's awaiting rebase.

david-driscoll · 2016-01-04T20:15:53Z

Will get working on this is in the next day or two, I've been enjoying some
much needed time off :)

On Mon, Jan 4, 2016 at 3:14 PM Ben Lesh notifications@github.com wrote:

Marking "Blocked" as it's awaiting rebase.

—
Reply to this email directly or view it on GitHub
#1077 (comment).

Thanks,
David

luisgabriel · 2016-01-05T01:14:27Z

@david-driscoll I recommend you use https://github.com/luisgabriel/RxJS/tree/pr-1077 as a base for updating this PR. A lot of the conflicts are already solved in this branch.

robwormald · 2016-01-05T07:55:49Z

👍 also @Blesh, who's the masochist now? #517 (comment)

benlesh · 2016-01-05T19:56:39Z

@robwormald haha. Yeah, that was before I knew it would reduce the amount of time I'd have to wait for builds. 📦

david-driscoll · 2016-01-09T19:23:36Z

@luisgabriel sorry for the delay, life happens. Now fully rebased and building correctly.

luisgabriel · 2016-01-09T21:18:34Z

@david-driscoll No problem at all. Thank you for the effort! ;)

luisgabriel · 2016-01-09T21:29:47Z

src/operator/distinctUntilChanged.ts

-  return this.lift(new DistinctUntilChangedOperator(compare, keySelector));
+export function distinctUntilChanged<T>(compare?: (x: T, y: T) => boolean): Observable<T>;
+export function distinctUntilChanged<T, K>(compare: (x: K, y: K) => boolean, keySelector?: (x: T) => K): Observable<T>;
+export function distinctUntilChanged<T, K>(compare: (x: K, y: K) => boolean, keySelector?: (x: T) => K): Observable<T> {


Is this correct?

If you supply a keySelector, then the values that get handed to compare, are the type that is returned by the keySelector otherwise the values handed to compare are T.

Got it. Thanks.

luisgabriel · 2016-01-10T01:17:50Z

LGTM 👍

kwonoj · 2016-01-10T23:40:19Z

Cool, I'm checking in this.. (finally!)

kwonoj · 2016-01-11T00:17:14Z

Merged with db7f0b1. Appreciate @david-driscoll for this huge effort, and also @luisgabriel takes effort to review this changes too.

lock · 2018-06-07T14:02:30Z

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 mentioned this pull request Dec 15, 2015

chore(typings): Enabled noImplictAny and updated typings #1050

Closed

david-driscoll force-pushed the all-types-no-types branch 2 times, most recently from a46c668 to 87e642e Compare December 15, 2015 21:54

luisgabriel reviewed Dec 15, 2015
View reviewed changes

david-driscoll force-pushed the all-types-no-types branch 2 times, most recently from c6de270 to c8e821c Compare December 15, 2015 22:17

luisgabriel reviewed Dec 15, 2015
View reviewed changes

david-driscoll force-pushed the all-types-no-types branch from c8e821c to c4608c7 Compare December 15, 2015 22:25

kwonoj reviewed Dec 15, 2015
View reviewed changes

benlesh added the blocked label Jan 4, 2016

kwonoj mentioned this pull request Jan 4, 2016

manual typings for bindCallback.create #1086

Closed

david-driscoll mentioned this pull request Jan 6, 2016

Type inference not working due to Observable<any> in switchmap #1139

Closed

chore(typings): enabled noImplictAny

db7f0b1

david-driscoll force-pushed the all-types-no-types branch 2 times, most recently from 2d0af04 to db7f0b1 Compare January 9, 2016 19:18

luisgabriel reviewed Jan 9, 2016
View reviewed changes

kwonoj merged commit db7f0b1 into ReactiveX:master Jan 11, 2016

kwonoj mentioned this pull request Jan 19, 2016

Typing fixes #1214

Closed

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

chore(typings): Enabled noImplictAny #1077

chore(typings): Enabled noImplictAny #1077

Conversation

david-driscoll commented Dec 15, 2015

kwonoj commented Dec 15, 2015

david-driscoll commented Dec 15, 2015

kwonoj commented Dec 15, 2015

david-driscoll commented Dec 15, 2015

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwonoj commented Dec 15, 2015

Choose a reason for hiding this comment

david-driscoll commented Dec 23, 2015

benlesh commented Dec 23, 2015

kwonoj commented Dec 23, 2015

luisgabriel commented Dec 23, 2015

trxcllnt commented Dec 25, 2015

kwonoj commented Dec 30, 2015

luisgabriel commented Dec 30, 2015

luisgabriel commented Dec 30, 2015

benlesh commented Jan 4, 2016

benlesh commented Jan 4, 2016

david-driscoll commented Jan 4, 2016

luisgabriel commented Jan 5, 2016

robwormald commented Jan 5, 2016

benlesh commented Jan 5, 2016

david-driscoll commented Jan 9, 2016

luisgabriel commented Jan 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisgabriel commented Jan 10, 2016

kwonoj commented Jan 10, 2016

kwonoj commented Jan 11, 2016

lock bot commented Jun 7, 2018