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

Completely remove RxJS dependency from the RxPlayer's source code. #1193

Merged
merged 8 commits into from
Dec 27, 2022

Conversation

peaBerberian
Copy link
Collaborator

This is one of the final (our demo pages still contains some RxJS code) instalments of our recurring PR to remove RxJS from the RxPlayer code (after #916, #962, #1042, #1127, #1091, #1135, #1158 and #1167).

With that PR, RxJS stops being a dependency of the RxPlayer code (but stays a "devDependency" for our demo pages for now, though we may want to switch to the more popular React-Redux webapp architecture in the future for popularity sake, as copiability is also one of its goals).

Just to give a last reminder of why we're doing this:

  1. To improve the approachability and readability of the RxPlayer code.

    The lazyness, non-shared by default, multiple emissions (which isn't linked to completion), complex combination operators, schedulers configuration and implicit cancellation nature of RxJS Observables are all very interesting features yet hard to get into and very easy to mess up.
    Forgetting to share may lead to multiple side-effects being performed when only one is needed (it happened more than once), looking why an Observable has been cancelled can be a very had task, handling Observables which may send events synchronously on subscription lead to a lot of special considerations (using specific schedulers if multiple subscriber exists, for one), lazyness make it harder to understand the performance of our application, and so on.

  2. Evoked in the previous point: to make the cancellation logic explicit.

    The RxPlayer performs a lot of asynchronous tasks (which is why we relied a lot on RxJS in the first place) which may have to be cancelled at some point (e.g. a request for a previously chosen quality, DRM negotiation when the content is changed buffer operations when the current track is changed etc.).

    With RxJS, the idiomatic way of handling cancellation is by stop listening to the Observable, which is a very interesting concept theoretically but in our usecases was too implicit, leading sometimes to issues (task cancelled too soon or not cancelled soon enough, necessity to play with RxJS subjects or create new Observables to be able to get the right timing etc.).

    Cancellation is now mostly handled by a TaskCanceller concept, which almost exactly copies the AbortController browser object (we may use an AbortController instead in the future? For now the main - and subtle - difference between the two is that the TaskCanceller also automatically generates a nice CancellationError on cancellation which can be very useful to communicate about cancellation through rejected promises).
    TaskCanceller-linked tasks are manually cancelled through a cancel call, and cancelling operations (such as clean-up) is done through an event listener on the task-side, this is both explicit and idiomatic for a JavaScript developper.

  3. To improve debuggability, notably by making call stacks great again.
    Due to lazyness and the operator-rich nature of RxJS, stack traces have never been exploitable. Improving those have been a subject for RxJS for a long time but by removing that library from the code, we can now obtain very inspectable call stacks

  4. An important chunks of our bugs are related to our handling of RxJS, some even linked to RxJS issues (we had for example 2 memory leaks from RxJS bugs, a long-lived issue this year - now-fixed - which led to many migraines and RxJS versions rollbacks, and multiple other issues those last years).

    Even if RxJS turns out with no bug in the future, its complexity is overkill for most of our use cases and in itself can lead to issues. Replacements in our situations can range from Promise to event emitters (or event-emitter likes, such as callbacks, TaskCanceller and SharedReference), which are generally more understood and led for now to less issues.

    The closest replacement of RxJS Observables we have now in our code is what we called SharedReference which can be compared to BehaviorSubjects: non-lazy structures wrapping some data, with the possibility to add an event listener to run some logic when that data changes. This is now the more advanced concept to grasp but it is still order of magnitudes simpler than RxJS Observables to get into.

  5. Observables was the default goto way of handling asynchronous data. Now, in most cases where there's only one emission, Promise are used instead.

    Promises are easier to combine to many browser API already relying on Promises, profit from async/await syntax, and is much more understood by JavaScript developpers than RxJS Observables.
    The main issue we could have with Promises is the obligation to have micro-tasks scheduled when awaiting them, but this is minor and generally lead to a more predictable flow.


The Stream part of the code now make heavy usage of callbacks given as parameters, used as event emitters.

This is the paradigm used instead of the more idiomatic EventTarget-like concept (with addEventListener and removeEventListener methods) to both allow synchronous events to be triggered right on call (without introducing a start/stop concept) and to better ensure - through TypeScript - that all events are either explicitely considered or skipped as all callbacks have to be defined on function call (which is a VERY nice feature). It also conveniently greatly facilitates bubbling up events, by just passing on to the corresponding callback, the reference to the parent callback to bubble-up too.

@peaBerberian peaBerberian added Refacto This Pull Request changes a lot of RxPlayer's code and/or logic Performance checks Performance tests are run on this issue / PR labels Dec 20, 2022
@peaBerberian peaBerberian force-pushed the misc/stream-no-rxjs branch 4 times, most recently from ef7d636 to a6f4f92 Compare December 21, 2022 10:51
@peaBerberian peaBerberian merged commit 72a9a8d into next Dec 27, 2022
@peaBerberian peaBerberian added this to the 3.30.0 milestone Dec 27, 2022
peaBerberian added a commit that referenced this pull request Jan 31, 2023
Completely remove RxJS dependency from the RxPlayer's source code.
peaBerberian added a commit that referenced this pull request Feb 8, 2023
Completely remove RxJS dependency from the RxPlayer's source code.
peaBerberian added a commit that referenced this pull request Feb 9, 2023
Completely remove RxJS dependency from the RxPlayer's source code.
@peaBerberian peaBerberian mentioned this pull request Feb 21, 2023
@peaBerberian peaBerberian mentioned this pull request Jun 13, 2023
@peaBerberian peaBerberian deleted the misc/stream-no-rxjs branch July 6, 2023 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance checks Performance tests are run on this issue / PR Refacto This Pull Request changes a lot of RxPlayer's code and/or logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant