Remove Subject.create, refactor WebSocketSubject #7381
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BREAKING CHANGE: Removed the deprecated `Subject.create` method. If you need to create an object that is "half Observable, half Observer", you'll need to either bolt `next`, `error`, and `complete` handlers onto an `Observable` and property type the return... or you'll need to create your own class that is backed by an `Observable`. In any case, if the `Observer` and the `Observable` are so unrelated that you have to bolt them together, you're probably better off with those two objects separately. This is why `Subject.create` has been deprecated for so long.
…input` and `output`.
BREAKING CHANGE: `WebSocketSubject` is no longer `instanceof Subject`. Check for `instanceof WebSocketSubject` instead.
benlesh
commented
Nov 21, 2023
benlesh
commented
Nov 21, 2023
benlesh
commented
Nov 21, 2023
ssilve1989
approved these changes
Nov 21, 2023
Comment on lines
+157
to
+167
| private _inputBuffer: In[] = []; | ||
|
|
||
| private _hasError = false; | ||
|
|
||
| private _error: any; | ||
|
|
||
| private _isComplete = false; | ||
|
|
||
| private _subscriberCounter = 0; | ||
|
|
||
| private _subscribers = new Map<number, Subscriber<Out>>(); |
There was a problem hiding this comment.
Should these be real private fields without the ugly _ prefix instead of TS private?
There was a problem hiding this comment.
Yeah. I think overall, RxJS is due for an overhaul in this area. The upside is we can't accidentally publicly expose any #private properties. The downside is you can't destructure them. Which is mostly a superficial downside.
There was a problem hiding this comment.
@benlesh, I might try to do this.
|
Thank you for the review @ssilve1989! ❤️ |
demensky
reviewed
Nov 22, 2023
| // Setting this here because a previous version of this allowed | ||
| // WebSocket to be polyfilled later than DEFAULT_WEBSOCKET_CONFIG | ||
| // was defined. | ||
| WebSocketCtor: WebSocket, |
There was a problem hiding this comment.
Will this throw an exception if the WebSocket is not available globally?
WebSocketCtor becomes almost useless.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
An incremental refactor of
WebSocketSubjectto clean it up quite a bit. best reviewed per commitAdds long-awaited type fix. WebSocketSubject can now take two type parameters that allow it to accept a different input type than it outputs:
Includes the following BREAKING CHANGES:
WebSocketSubjectis no longerinstanceof Subject. It really didn't make any sense to do this, it didn't leverage anything fromSubjectto begin with.Subject.create, a long-deprecated API has been removed. If you recall,Subject.create(observer, observable)basically "glued" the observer to the observable.This removes
AnonymousSubject, which was never exported, and was overall weird.