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: Update AbortSignal polyfill for new compiler #7176

Merged

Conversation

joeyparrish
Copy link
Member

@joeyparrish joeyparrish commented Aug 20, 2024

This upgrades the compiler and reworks the AbortSignal polyfill to match the new compiler externs for that class. This is important to make Shaka Player compatible with the latest compilers in use inside Google.

Note that the Closure compiler is deprecated, so this should be our final upgrade. We will some day move to TypeScript.

This does not update the Closure library, because the latest version causes failures we don't understand in the loading mechanism in test/test/boot.js.

@joeyparrish
Copy link
Member Author

Hrm... this one seems not to be ready yet.

I made the same change in our Google-internal repo and it worked, and I tested compiling this locally in context of this GitHub repo, but apparently there is a runtime issue with the tests. Something about the new compiler version vs how we load things in the tests, I presume.

Copy link
Member

@avelad avelad left a comment

Choose a reason for hiding this comment

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

Please, review #7149 (it removes the polyfill)

This upgrades the compiler and reworks the AbortSignal polyfill to
match the new compiler externs for that class.  This is important to
make Shaka Player compatible with the latest compilers in use inside
Google.

Note that the Closure compiler is deprecated, so this should be our
final upgrade.  We will some day move to TypeScript.

This does _not_ update the Closure library, because the latest version
causes failures we don't understand in the loading mechanism in
test/test/boot.js.
@joeyparrish joeyparrish force-pushed the abort-signal-polyfill-vs-closure branch from 079194b to 57ad01e Compare August 20, 2024 17:58
@joeyparrish
Copy link
Member Author

I have fixed this PR by undoing the upgrade of the Closure library specifically. This was the source of the incompatibility with our test infra, specifically test/test/boot.js. The Closure compiler and deps system have still been upgraded, which brings the open source build in line with Google's internal systems.

Please, review #7149 (it removes the polyfill)

I will have to review #7149 carefully, and there may be some risk. In contrast to that, this fix doesn't change behavior and will allow me to fix syncing source code to our internal systems right away. Also, the compiler upgrade will keep the open source releases from accidentally breaking Google's internal systems again in the future (since now they will be using the same compiler). Therefore I would like to merge this first.

Happy to discuss it more if there are any objections. And I promise to review #7149 today or tomorrow.

@shaka-bot
Copy link
Collaborator

Incremental code coverage: 73.33%

@joeyparrish joeyparrish merged commit c97e689 into shaka-project:main Aug 20, 2024
12 of 15 checks passed
@joeyparrish joeyparrish deleted the abort-signal-polyfill-vs-closure branch August 20, 2024 18:19
joeyparrish added a commit that referenced this pull request Aug 20, 2024
This upgrades the compiler and reworks the AbortSignal polyfill to match
the new compiler externs for that class. This is important to make Shaka
Player compatible with the latest compilers in use inside Google.

Note that the Closure compiler is deprecated, so this should be our
final upgrade. We will some day move to TypeScript.

This does _not_ update the Closure library, because the latest version
causes failures we don't understand in the loading mechanism in
test/test/boot.js.
joeyparrish added a commit that referenced this pull request Aug 20, 2024
This upgrades the compiler and reworks the AbortSignal polyfill to match
the new compiler externs for that class. This is important to make Shaka
Player compatible with the latest compilers in use inside Google.

Note that the Closure compiler is deprecated, so this should be our
final upgrade. We will some day move to TypeScript.

This does _not_ update the Closure library, because the latest version
causes failures we don't understand in the loading mechanism in
test/test/boot.js.
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Oct 19, 2024
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Oct 19, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants