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

chore: Remove state engine #5752

Merged
merged 42 commits into from
Oct 31, 2023
Merged

Conversation

theodab
Copy link
Contributor

@theodab theodab commented Oct 9, 2023

The state engine mechanism, designed for the player class, was over-engineered. The structure of the class makes debugging player errors unnecessarily annoying, by obfuscating the code-path the error followed, and in general
has created a significant amount of technical debt.
This changes the player to use an async-await setup for the top-level operations, laying things out much more cleanly
and linearly.

@theodab theodab added the type: code health A code health issue label Oct 9, 2023
@theodab theodab marked this pull request as draft October 9, 2023 08:05
@theodab
Copy link
Contributor Author

theodab commented Oct 9, 2023

So this is still a work in progress. I'll need to do a lot more manual testing to make sure things aren't broken, I need to re-write the JSDoc of a lot of functions so they no longer mentions the state engine, and I'll definitely need to update a lot of tests.
I'm publishing this in its current state to see if people think this approach looks like it has merit.

@theodab theodab requested review from joeyparrish and avelad October 9, 2023 08:06
@avelad avelad added this to the v4.6 milestone Oct 9, 2023
lib/player.js Outdated Show resolved Hide resolved
The state engine mechanism, designed for the player class, was
over-engineered. The structure of the class makes debugging player
errors unnecessarily annoying, by obfuscating the code-patch the
error followed.
This changes the player to use an async-await setup for the
top-level operations, laying things out much more clearly and
linearly.
@theodab theodab force-pushed the noMoStateEngineBranch branch from 1268ed1 to edb3d72 Compare October 10, 2023 06:09
lib/player.js Outdated Show resolved Hide resolved
lib/player.js Outdated Show resolved Hide resolved
@shaka-bot
Copy link
Collaborator

shaka-bot commented Oct 10, 2023

Incremental code coverage: 91.12%

@theodab
Copy link
Contributor Author

theodab commented Oct 10, 2023

It looks like the tests hang forever, probably because of the new mutexes. Just canceling the workflow for now to avoid wasting time, and I'll look into the tests on my local machine.

@theodab theodab marked this pull request as ready for review October 17, 2023 08:59
@theodab theodab force-pushed the noMoStateEngineBranch branch from d18d5c8 to 0b2fca6 Compare October 26, 2023 20:32
test/player_unit.js Outdated Show resolved Hide resolved
@theodab theodab merged commit 4425dca into shaka-project:main Oct 31, 2023
@theodab theodab deleted the noMoStateEngineBranch branch October 31, 2023 06:59
avelad pushed a commit that referenced this pull request Oct 31, 2023
Looks like this was just introduced in #5752
Robloche pushed a commit to Robloche/shaka-player that referenced this pull request Nov 30, 2023
The state engine mechanism, designed for the player class, was
over-engineered. The structure of the class makes debugging player
errors unnecessarily annoying, by obfuscating the code-path the error
followed, and in general
has created a significant amount of technical debt.
This changes the player to use an async-await setup for the top-level
operations, laying things out much more cleanly
and linearly.

---------

Co-authored-by: Álvaro Velad Galván <ladvan91@hotmail.com>
Robloche pushed a commit to Robloche/shaka-player that referenced this pull request Nov 30, 2023
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Dec 30, 2023
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Dec 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: code health A code health issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants