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

[WIP] remove RxJS from the SegmentFetchers #1158

Merged
merged 3 commits into from
Sep 14, 2022

Conversation

peaBerberian
Copy link
Collaborator

@peaBerberian peaBerberian commented Sep 12, 2022

This is yet another refacto to remove RxJS from another part of the code (a multi-year effort after #916, #962, #1042, #1127, #1091 and #1135) this time it removes it both from:

  • the "SegmentFetcher", which is the part scheduling segment requests.
  • the VideoThumbnailLoader experimental tool, because it now mainly contained conversions of promise-based code to RxJS-based code to Promise-based code!

Those parts were tricky to port, this branch even being initially pretty old (more than 6 months for sure, maybe much more), due to the event-rich and callback-rich nature of it (full of potential re-entrancy bugs and possible race conditions, even more now that there is Promise involved and the corresponding microtask concerns), but I think that we reached a state were it seems to work well.

It is based on the rewrite of what was previously called the ObservablePrioritized but is now called TaskPrioritizer whose work started here: #1030, one of the more tricky utils to rewrite in the RxPlayer due to a lot of synchronous and asynchronous things to consider (our interview test for senior developper is even based on it :p!).

I still have to re-read everything, perhaps remove some unnecessary code, and add some tests.

As a reminder we're trying to remove reliance on RxJS globally for various reasons, which could be resumed as:

  1. code approachability (RxJS operators and the lazy-by-default behavior are already hard to grasp, but when you add schedulers, sharing, subjects, replaying etc., it becomes a nightmare sometimes only understandable - poorly - by its creator).
  2. to have a more explicit cancellation logic (Right now, knowing why a request was cancelled, or why the player just decided to stop doing something is very hard to understand, as it happens implicitely when the corresponding Observables are unsubscribed from)
  3. ease of debugging (call stacks are most notably inexploitable with RxJS)
  4. Removal of long-lived RxJS issues due to the fact that we're using Observables sometimes for mixed synchronous/asynchronous stream of events, where RxJS doesn't excel

@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 Priority: 3 (Low) This issue or PR has a low priority. labels Sep 12, 2022
@peaBerberian peaBerberian force-pushed the misc/segment-fetcher-no-rxjs branch 2 times, most recently from 0d0b7f9 to d578aa4 Compare September 13, 2022 13:11
@peaBerberian peaBerberian merged commit 044f145 into next Sep 14, 2022
peaBerberian added a commit that referenced this pull request Sep 15, 2022
[WIP] remove RxJS from the SegmentFetchers
@peaBerberian peaBerberian added this to the 3.29.0 milestone Sep 16, 2022
@peaBerberian peaBerberian deleted the misc/segment-fetcher-no-rxjs branch July 6, 2023 12:06
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 Priority: 3 (Low) This issue or PR has a low priority. 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.

None yet

2 participants