Skip to content

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

Merged
benlesh merged 3 commits intoReactiveX:7.xfrom
raveclassic:raveclassic/fix_subscriber_next
Jun 16, 2023
Merged

fix(subscriber): strict type signature for next method#7172
benlesh merged 3 commits intoReactiveX:7.xfrom
raveclassic:raveclassic/fix_subscriber_next

Conversation

@raveclassic
Copy link

@raveclassic raveclassic commented Jan 25, 2023 •
edited
Loading

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
@raveclassic
fix(subscriber): strict Subscriber.next type signature
ba2b0b1 
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 •
edited
Loading

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 🌴

benlesh
benlesh requested changes Jan 26, 2023
View reviewed changes
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

raveclassic commented Jan 31, 2023

@benlesh tests are fixed 🤌

@raveclassic raveclassic requested a review from benlesh January 31, 2023 10:08
@raveclassic
Copy link
Author

raveclassic commented Feb 18, 2023

@benlesh could you review, please?

@raveclassic
Copy link
Author

raveclassic commented Mar 13, 2023

@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

raveclassic commented Jun 15, 2023

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

jakovljevic-mladen commented Jun 16, 2023

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

@raveclassic
Copy link
Author

raveclassic commented Jun 19, 2023

@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.

@benlesh benlesh mentioned this pull request Jun 19, 2023
Merged
@dgadelha dgadelha mentioned this pull request Apr 7, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benlesh benlesh benlesh approved these changes

+1 more reviewer

@LcsGa LcsGa LcsGa approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@raveclassic @benlesh @LcsGa @jakovljevic-mladen