chore(typings): Enabled noImplictAny and updated typings #1050
Conversation
david-driscoll
force-pushed
the
all-types-without-extras
branch
from
e57d9be
to
4d9d33d
Compare
Current timesES6Files: 280 AMDFiles: 249 CommonJSFiles: 281 New TimesES6Files: 281 AMDFiles: 250 CommonJSFiles: 282 |
With this change compliation time and performance is improved
david-driscoll
force-pushed
the
all-types-without-extras
branch
from
4d9d33d
to
136f97f
Compare
|
I personally like this changes and was considered quite while ago, wasn't sure if this is feasible for current codebases yet. Again, generally I prefer this idea to be strict manner. Few suggestion for changes though,
May have some comment in each code as well.. and cc @Blesh , @staltz here also since this'll impact code styles from now on expect opinions. |
| export function isPromise<T>(value: any | Promise<T>): value is Promise<T> { |
There was a problem hiding this comment.
Would you able to share some details of
- what does generic does do?
- reason to change return type from boolean?
There was a problem hiding this comment.
The return type still behaves as a boolean, but it also acts as a type guard as well.
Given the following:
var value: Observable<T> | Promise<T>
if (isPromise(value)) {
// to the type system value is now just Promise<T>
} else {
// to the type system value is now just Observable<T>
}
|
@kwonoj I totally expect opinions, hence testing the waters first. 😄 As far as I tried to make the changes as non-invasive as possible, let me know if you notice something left behind. No actual code changes, just the types. The one thing that impressed me was the drastic drop in memory usage, and the drastic reduction in types that were being tracked by the compiler. |
| @@ -0,0 +1,18 @@ | |||
| import {Observable} from './Observable'; | |||
There was a problem hiding this comment.
I think this file (types.ts) is not landed in current TOT of codebase, isn't it?
|
@david-driscoll Thanks for effort, this requires lot of changes. :) One another suggestion in addition to above, I could see some changes like Given circumstances of PR itself is already huge due to nature of changes, I'd like to suggest to isolate PR as much as possible to not include directly related changes. Those refactoring can be separated into another PR, (maybe I presume too early?) |
|
I'll hold on to add comment into each individual files for now to gather opinions regarding essence of this PR first. Once it's agreed to land this, detail can be updated from that point. |
|
I left in types on purpose because of the usefulness of the common types for handing complicated declaration. In some cases we see method signatures like |
: Yes, I do not deny those and I agree in general, suggestion is simply better to have separate PR for those. Once you're able to create another PR for those single changes only, that can be easily discussed in checked in, while this PR is on discussion. :) In this PR currently, it contains 2 refactoring of
and first change is quite huge, mixed discussion in this single PR could possibly delay check in of 2nd as well, and reviewer also requires additional effort (to discuss types.ts, have to open changes tab has >100 files). Please consider it as suggestion for purely logistics perspective. |
|
Could you update the subject of the commit message to use lower case and present tense? Just a minor detail to keep the same pattern as the others commits in the history. ;) |
|
I am in general favorable of merging this PR, but I haven't reviewed all the changes since there are many. |
Same. |
|
OK, think we're in consensus. I'll drive initial effort for in-detail review for changes, and will bring in @Blesh , @staltz later before finally merge it. Before proceed further comments, @david-driscoll would you kindly separate PR between 2 I commented above (#1050 (comment))? |
| @@ -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, | |||
There was a problem hiding this comment.
What about K instead of TKey?
It would keep the single letter pattern. It would also keep it similar to the signature used by RxJava.
|
There are a lot of good stuff in this PR. But I agree with @kwonoj here. This PR is too big to be reviewed properly. I also have mixed feeling about some changes. For example: - if (result === errorObject) {
- return new ErrorObservable(errorObject.e);
+ if (result as any === errorObject) {
+ return new ErrorObservable<any>(errorObject.e);-const observableProto = (<KitchenSinkOperators<any>>Observable.prototype);
+const observableProto = (<KitchenSinkOperators<any>><any>Observable.prototype);-Observable.prototype.exhaust = exhaust;
+(Observable.prototype as any).exhaust = exhaust;I think this kind of change adds verbosity without many gains. |
|
I'll put on hold reviewing this PR and wait for separation of changes for now. |
|
@kwonoj I'll see if I have time tonight to gets things apart, might take a little bit of work. - if (result === errorObject) {
- return new ErrorObservable(errorObject.e);
+ if (result as any === errorObject) {
+ return new ErrorObservable<any>(errorObject.e);In the context of the method it makes sense, we could instead say it's of type T to match the return value. The big thing is errors in Rx aren't the same type as the observable might have contextually, in fact it adds value here, we're saying the error can actually be anything. -const observableProto = (<KitchenSinkOperators<any>>Observable.prototype);
+const observableProto = (<KitchenSinkOperators<any>><any>Observable.prototype);This could just be converted to -Observable.prototype.exhaust = exhaust;
+(Observable.prototype as any).exhaust = exhaust;This is again just a typing that can be corrected, I just hadn't corrected it. exhaust was conceptually moved to the extended space, but still exists on the base Observable. |
I see the value of making the type parameter of
Much better. ;) |
|
best we can do is I just thought the as any syntax was easier to write/type than |
|
Yeah, I was not questioning the casting syntax, but the casting itself. I prefer not to do casting in situations such as this one. But I suppose it is needed to enable |
|
we could make the |
|
Marked as blocked for now to prevent accidental merging, #1077 should be resolved first. |
|
@david-driscoll , I think #1086 is end conclusions for this PR's orignally aimed for and this can be closed by checking ing #1086 - is my understanding correct? |
|
Yeah, we should be good for this issue. |
|
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. |
With this change compliation time and performance is improved
Testing the waters a little bit here, but this change doesn't include any of the massive changes to how the external typings are generated.