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

Subjects refactoring #4942

Draft
wants to merge 10 commits into
base: master
from
Draft

Conversation

@BioPhoton
Copy link
Contributor

BioPhoton commented Aug 3, 2019

Description:
Refactoring of Subjects logic. Detailed info in the related issue.

Related issue (if exists):
#4880

BioPhoton added 10 commits Jun 20, 2019
Following things could get refactored:
- Subject.ts
  - Referencing properties over `this` instead of destruction `this`.
  - First-class functions instead loops
  - Getting rid of `Symbol` usage
- BehaviorSubject.ts
  - We could remove typing
- ReplaySubject.ts
  - Replacing `ReplayEvent` class with an interface

More details in #4880
include copying of observers array

More details in #4880
SubjectSubscriber workaround

More details in #4880
Reference class properties over `this`

More details in #4880
Fixed imports

More details in #4880
Fixed typing

More details in #4880
…cts-refactoring

# Conflicts:
#	src/internal/Subject.ts
#	src/internal/util/toSubscriber.ts
@@ -40,6 +39,7 @@ export class BehaviorSubject<T> extends Subject<T> {
}

next(value: T): void {
super.next(this._value = value);
this._value = value;
super.next(this._value);

This comment has been minimized.

Copy link
@benlesh

benlesh Aug 9, 2019

Member

More verbose. Should revert.

@@ -35,19 +41,18 @@ export class ReplaySubject<T> extends Subject<T> {
}

private nextInfiniteTimeWindow(value: T): void {
const _events = this._events;

This comment has been minimized.

Copy link
@benlesh

benlesh Aug 9, 2019

Member

This was done to prevent additional property lookups and the change should be reverted.

@@ -71,18 +73,22 @@ export class ReplaySubject<T> extends Subject<T> {
subscription = new SubjectSubscription(this, subscriber);
}

if (scheduler) {
subscriber.add(subscriber = new ObserveOnSubscriber<T>(subscriber, scheduler));
if (this.scheduler) {

This comment has been minimized.

Copy link
@benlesh

benlesh Aug 9, 2019

Member

Additional property lookups should be reverted.

subscriber.next(<T>_events[i]);
}
if (this._infiniteTimeWindow) {
_events.forEach(event => {

This comment has been minimized.

Copy link
@benlesh

benlesh Aug 9, 2019

Member

for loop is more efficient, particularly at processing arrays. forEach requires a function allocation.

copy[i].next(value);
}
this.observers.slice()
.forEach(observer => observer.next(value));

This comment has been minimized.

Copy link
@benlesh

benlesh Aug 9, 2019

Member

revert. for loop is more efficient.

@benlesh

This comment has been minimized.

Copy link
Member

benlesh commented Aug 9, 2019

@BioPhoton, I know that this is just a draft, and I'm really sorry to say this, but we can't accept really any of these changes. Most of these changes are removing optimizations that we had done on purpose, for example using for loops to iterate over arrays, and pulling properties into variables to prevent repeated property access.

:) I do appreciate the thought though. <3

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.