Skip to content

refactor(types): enable strict fntype compiler option #3050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 8, 2018

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Nov 5, 2017

Description:
This PR enables strictFnType in master branch resolves build error with 2.6 version of tsc. (related with #3031) . due to our inheritance structure, there are some types are actually loosened (mostly around Action's work types).

This is targeting master intentionally due to scope of change is relatively large, include (probably) unnecessary part to close issue itself. Some change need to be cherrypicked into stable though.

Related issue (if exists):

@rxjs-bot
Copy link

rxjs-bot commented Nov 5, 2017

Messages
📖

CJS: 1363.7KB, global: 731.1KB (gzipped: 118.0KB), min: 141.3KB (gzipped: 30.8KB)

Generated by 🚫 dangerJS

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.204% when pulling 2b158ba on kwonoj:strict-fn into 8275cef on ReactiveX:master.

@coveralls
Copy link

coveralls commented Nov 6, 2017

Coverage Status

Coverage remained the same at 97.204% when pulling 2b158ba on kwonoj:strict-fn into 8275cef on ReactiveX:master.

@kwonoj kwonoj requested a review from benlesh November 12, 2017 19:03
@bondz
Copy link

bondz commented Dec 4, 2017

I understand eveyone is busy, but can we get this reviewed early? I understand there's a workaround but its suboptimal.

@kwonoj
Copy link
Member Author

kwonoj commented Dec 4, 2017

I forgot this :/ and I'm sure this'll require to resolve merge conflict with #3145.

Anyway, this is for v6 branch and not for stable, for 5.x probably we need small scoped change only resolve issue.

@bondz
Copy link

bondz commented Dec 4, 2017

Oh, okay, thanks for clarifying.

@samuelhnrq
Copy link

Well the new year is here, and as I see it the #3145 that is causing the conflicts only moves files around, so what is needed to do is move the files this PR changed into the new locations? I really agree this should be also back-ported into 5.x, this issue breaks rxjs completely in typescript 2.6.1

@kwonoj
Copy link
Member Author

kwonoj commented Jan 18, 2018

uh-oh, this PR itself even have huge conflict due to recent changes. I need to rebase this first, then picks some into 5.5

@@ -177,11 +177,11 @@ class WindowTimeSubscriber<T> extends Subscriber<T> {
if (windowCreationInterval !== null && windowCreationInterval >= 0) {
const closeState: CloseState<T> = { subscriber: this, window, context: <any>null };
const creationState: CreationState<T> = { windowTimeSpan, windowCreationInterval, subscriber: this, scheduler };
this.add(scheduler.schedule(dispatchWindowClose, windowTimeSpan, closeState));
this.add(scheduler.schedule(dispatchWindowCreation, windowCreationInterval, creationState));
this.add(scheduler.schedule<CloseState<T>>(dispatchWindowClose, windowTimeSpan, closeState));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This smells a little. I feel like all of this should be cleaner.

@benlesh benlesh merged commit 94156a2 into ReactiveX:master Feb 8, 2018
@benlesh
Copy link
Member

benlesh commented Feb 8, 2018

:shipit: Good add, @kwonoj!

@lock
Copy link

lock bot commented Jun 6, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
@kwonoj kwonoj deleted the strict-fn branch October 4, 2019 05:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants