Skip to content
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

fix(subscriber): strict type signature for next method #7172

Merged
merged 3 commits into from
Jun 16, 2023

Conversation

raveclassic
Copy link

@raveclassic raveclassic commented Jan 25, 2023

Description:

This PR fixes type signature of Subscriber.prototype.next method that was previously taking optional argument which was violating generic constraint T.

BREAKING CHANGE: Subscriber.prototype.next now takes strict argument.

new Observable(subscriber => {
  subscriber.next() // throws in compile time (expected, caused by this PR)
})

new Observable<string>(subscriber => {
  subscriber.next() // throws in compile time (expected, caused by this PR)
})

new Observable<void>(subscriber => {
  subscriber.next() // ok
})

Related issue (if exists): #2852

Additional changes:

  • update package version in lockfile to 7.8.0 (running npm i)

@raveclassic raveclassic changed the title Raveclassic/fix subscriber next fix(subscriber): strict type signature for next method Jan 25, 2023
BREAKING CHANGE: Subscriber.next now takes strict argument
@raveclassic raveclassic force-pushed the raveclassic/fix_subscriber_next branch from 510ab94 to ba2b0b1 Compare January 25, 2023 14:11
@benlesh
Copy link
Member

benlesh commented Jan 25, 2023

Yay! I almost forgot we were waiting for v8 for this one.

@raveclassic
Copy link
Author

raveclassic commented Jan 25, 2023

Awesome! Thanks for approving!
Seems I have to fix some things, missed that. Will come back to this PR when I return back to my laptop in a few days 🌴

Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

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

Uh oh. We have broken tests.

@raveclassic
Copy link
Author

@benlesh tests are fixed 🤌

@raveclassic
Copy link
Author

@benlesh could you review, please?

@raveclassic
Copy link
Author

@benlesh just a friendly reminder ;) would love this to land in rxjs@7 as now our codebase heavily relies on new Observable(subscriber => {}) notation and we'd love to have this fixed 🙏

@raveclassic
Copy link
Author

Folks? :D @benlesh @LcsGa

@LcsGa
Copy link

LcsGa commented Jun 15, 2023

Hello! Sorry, I can't do more I'm not in the team 😅

@benlesh benlesh merged commit 0e2ef5e into ReactiveX:7.x Jun 16, 2023
@raveclassic raveclassic deleted the raveclassic/fix_subscriber_next branch June 16, 2023 09:18
@jakovljevic-mladen
Copy link
Member

@benlesh, is this merged to 7.x by mistake or on purpose?

@raveclassic
Copy link
Author

@jakovljevic-mladen @benlesh I expected this to land on 7.x 🤔

@benlesh
Copy link
Member

benlesh commented Jun 19, 2023

@jakovljevic-mladen honestly, I didn't notice, but I'm generally okay with this as a "fix" in 7.x. I'll go back and have a look to double check, but I think this is a valid bug fix, as people should not be calling next() on a Subscriber<number> or something. Type safety is important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants