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.

Sign up for GitHub

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

Jump to bottom

The Subscriber refactor, operator updates, and scheduling fixes. #1106

Closed
wants to merge 14 commits into from
Closed

The Subscriber refactor, operator updates, and scheduling fixes. #1106

wants to merge 14 commits into from

Conversation

trxcllnt
Copy link
Member

attention: @Blesh @staltz @kwonoj

  1. Factors out the shared Subscription list from Subscribers and updates the Observable, Subject, and Operator implementations accordingly.
  2. Refactors some operators that manage inner Subscriptions but weren't extending OuterSubscriber or using our subscribeToResult util. The more operators that use this pattern, the more operators that will interop with Array/Iterable/Promise/Observable-esque instances.
  3. Includes fixes to our Scheduler Action implementations to ensure:
    • Actions scheduled to occur on the next tick still execute if the first scheduled action is canceled
    • Asap/Queued Actions that reschedule with a delay fallback to using setTimeout instead of rescheduling like they normally would.
  4. fixes ReplaySubject to route messages through its scheduler (if specified), which mimics current RxJS v4 behavior.
  5. fixes Observables and operators that default to the queue scheduler. Nothing should ever default to the queue scheduler, because
    • our Observables fall back to iterative loops if unspecified
    • timing operators that don't ensure their relative times are greater than zero will schedule and execute their logic synchronously. While not technically a bug, this is a horrible thing to do to an end user. Default to asap scheduler for timing operators (like delay).
  6. Ensures all tests are run asynchronously, because Jasmine has difficulty with the boundaries between synchronous and asynchronous tests, and will non-deterministically fail for no reason.
  7. Style updates:
    • null instead of void 0 expressions
    • use our utility functions in more places
    • use more inherited properties instead of redefining them (in Subjects, operators)
    • call global functions from the root (root.setTimeout instead of setTimeout)

@trxcllnt
refactor(observable): moves Symbol.observable definition to the bottom.
7065c46 
It confuses Sublime's TypeScript plugin.
@trxcllnt
refactor(Subscription): Cleans up Subscription type checking.
5092478
@trxcllnt
refactor(Observable): Move Subscription resolution to util file.
a51173d 
Cleans up Observable properties, and only creates a new Subscriber if the Observable has an operator.
@trxcllnt
refactor(operator): Add default operator implementation.
dc5f131
@trxcllnt
refactor(observer): Add static empty Observer implementation.
ab8a858
@trxcllnt
refactor(subscriber): Refactor Subscriber's shared Subscription list.
af156f9 
Remove shared Subscription list. Make Subscribers automatically unsubscribe upon error or completion. Add SafeSubscriber to safely wrap callback arguments to Observable's subscribe.
@trxcllnt
refactor(subjects): Update Subject implementations for new Subscripti…
cea0871 
…on management.

Updates Subject, AsyncSubject, BehaviorSubject, and ReplaySubject to mimic the behavior of Subscriber. Removes the private BidirectionalSubject in favor of implementing the chaining logic in the Subject's methods (this allows us to extend and chain Subjects with `lift` in the same manner as we can with Observable).

Fixes ReplaySubject to route messages through its scheduler (if specified), which mimics current RxJS v4 behavior.

Refactors Subject's Symbol.rxSubscriber implementation to return the Subject wrapped inside a Subscriber. This ensures the Subject won't be disposed if its Subscription is.
@trxcllnt
refactor(promises): Update from/fromPromise/subscribeToResult.
c962f41 
Use isPromise utility function to check if an instance is a Promise. Import and use `root.setTimeout` instead of using global `setTimeout` definition.
@trxcllnt
style(nulls): Replace void 0 expressions with null.
1ea4617
@trxcllnt
refactor(observables): Updates Observable _subscribe type signatures …
5664050 
…for new Subscriber impl.
@trxcllnt
refactor(operators): fix methods that incorrectly default to use the …
a56e32c 
…queue Scheduler.
@trxcllnt
refactor(operators): Update operators per new Subscription chaining.
973155c 
Updates most operators to correctly extend the new Subscriber implementation. Refactors some operators that track inner Subscriptions to extend OuterSubscriber. Updates the test-helper to ensure all tests are run asynchronously, as Jasmine has trouble with the boundaries between synchronous and asynchronous tests.
@trxcllnt
fix(scheduling): Fixes bugs in scheduled actions.
863f96f 
Fix AsapAction to store its scheduled ID on the scheduler instead of on itself. This ensures that if the first AsapAction is canceled, other AsapActions will still execute.

Ensure both QueueAction and AsapAction extend FutureAction, so if either action is rescheduled with a delay > 0, they'll successfully reschedule with the proper timeout.
@trxcllnt
fix(ReplaySubject): Fix case-sensitive import.
f80328e
@benlesh
Copy link
Member

benlesh commented Dec 25, 2015

FWIW: I've already reviewed most of this over the last few days. I'll give it a final review over the next few days. It's Christmas, so it's probably going to be more later than sooner.

@trxcllnt trxcllnt mentioned this pull request Dec 25, 2015
Merged
@benlesh
Copy link
Member

benlesh commented Dec 29, 2015

Also this commit is a fix, not a refactor.

We'll need to update the CHANGELOG manually with what's been fixed, because the changes required to fix those issues were broad, required multiple commits, and weren't necessarily linked directly to those issues

@benlesh
Copy link
Member

benlesh commented Dec 29, 2015

Awesome work, @trxcllnt... as always. This is merged as of de31f32...

Note: @kwonoj don't let me forget to update the CHANGELOG manually (or perhaps add a commit to do so) so the fixes for other issues are shown.

@benlesh benlesh closed this Dec 29, 2015
@kwonoj
Copy link
Member

kwonoj commented Dec 30, 2015

OK, will keep eye ๏_๏ on @Blesh to see CHANGELOG is coming, send some poke around if it doesn't arrive. (feeling having separate commit would be feasible for this case?)

@trxcllnt trxcllnt deleted the subscription-refactor branch December 30, 2015 01:53
@lock
Copy link

lock bot commented Jun 7, 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 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
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
@trxcllnt @benlesh @kwonoj @staltz