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

fix(net): Remove AbortController polyfill #7149

Merged
merged 5 commits into from
Aug 21, 2024

Conversation

tykus160
Copy link
Member

@tykus160 tykus160 commented Aug 9, 2024

AbortController polyfill attaches to the global object and it messes up with feature detection for projects that are using shaka. As it is not able to properly abort ongoing requests, it doesn't give much value.

@avelad
Copy link
Member

avelad commented Aug 9, 2024

@shaka-bot test

@shaka-bot
Copy link
Collaborator

@avelad: Lab tests started with arguments:

  • pr=7149

@shaka-bot
Copy link
Collaborator

shaka-bot commented Aug 9, 2024

Incremental code coverage: 97.22%

@avelad
Copy link
Member

avelad commented Aug 9, 2024

i think that is necessary for hls parser, @joeyparrish i’m on holidays, can you review it? thanks!

@avelad avelad requested a review from joeyparrish August 9, 2024 15:50
@tykus160
Copy link
Member Author

Ah sorry, I missed that. My point still stands though - shaka has network abstraction layer and PendingRequest class inherits AbortableOperation with an abort() method. We should use this in that case, not AbortController. I'll try to update this PR.

@avelad
Copy link
Member

avelad commented Aug 19, 2024

@shaka-bot test

@shaka-bot
Copy link
Collaborator

@avelad: Lab tests started with arguments:

  • pr=7149

@avelad avelad added type: bug Something isn't working correctly priority: P2 Smaller impact or easy workaround component: networking The issue involves the networking system of Shaka Player labels Aug 19, 2024
@avelad avelad added this to the v4.11 milestone Aug 19, 2024
@avelad avelad requested review from avelad and theodab August 20, 2024 09:01
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure whether or not this is a mistake. Please take a look!

@joeyparrish
Copy link
Member

The merge conflicts are trivial. I changed abort_controller.js in #7176 for an easy cherry-pick. You can just delete the updated version and merge.

@joeyparrish
Copy link
Member

@tykus160, I have prepared new releases for v4.10.x and v4.9.x, but I will wait another day to see this get finished and merged.

Copy link
Contributor

@theodab theodab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this mostly looks good (besides Joey's comment).

@avelad avelad requested review from theodab and joeyparrish August 21, 2024 09:12
@joeyparrish joeyparrish merged commit 65e6681 into shaka-project:main Aug 21, 2024
14 of 17 checks passed
@tykus160 tykus160 deleted the wt-no-ac-polyfill branch August 25, 2024 06:46
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Oct 20, 2024
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Oct 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: networking The issue involves the networking system of Shaka Player priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants