Skip to content

Conversation

@michaelgiraldo
Copy link

@michaelgiraldo michaelgiraldo commented Dec 28, 2025

Supersedes #24679.

Context

  • Closure 20251216 updates externs/typing, which triggers -Werror=closure failures around Atomics.waitAsync (Promise typing) and makes some local externs redundant.

Changes

  • Bump google-closure-compiler to 20251216.0.0.
  • Remove duplicate externs now provided by Closure (WebAssembly.Global, Symbol.dispose).
  • Adjust Atomics.waitAsync extern typing and add targeted @Suppress {checkTypes} where Promise.value is consumed (libatomic/libpthread/libwasm_worker).
  • Rebaseline codesize expectations after the compiler update.

Tests

  • source /path/to/emsdk_env.sh && EM_CONFIG=/path/to/.emscripten test/runner --rebaseline codesize --cores=1
  • source /Users/mg/emdsdk_google/emsdk/emsdk_env.sh && EM_CONFIG=/Users/mg/emdsdk_google/emsdk/.emscripten EMTEST_BROWSER='"/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" --enable-unsafe-webgpu --enable-features=WebGPU --use-angle=metal' test/runner --cores=1 --no-browser-auto-config browser_2gb.test_webgpu_basic_rendering_closure

Notes

  • Tested on macOS arm64 with emsdk sdk-main-64bit built from source.
  • Codesize diffs are expected with the Closure version bump.
  • Local run warns about missing posixtestsuite submodule; codesize suite does not depend on it.

Copilot AI review requested due to automatic review settings December 28, 2025 05:54
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the google-closure-compiler dependency from version 20240317.0.0 to 20251216.0.0 and removes duplicate extern definitions that are now provided natively by the newer Closure Compiler version.

  • Updates google-closure-compiler to version 20251216.0.0
  • Removes duplicate externs for Atomics.waitAsync and WebAssembly.Global
  • Updates transitive dependencies including vinyl (2.2.1 → 3.0.1) and replace-ext (1.0.1 → 2.0.0)

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
src/closure-externs/closure-externs.js Removes duplicate extern definitions for Atomics.waitAsync and WebAssembly.Global that are now provided by the updated Closure Compiler
package.json Updates google-closure-compiler version from 20240317.0.0 to 20251216.0.0
package-lock.json Updates google-closure-compiler and its dependencies, including new platform-specific binaries (linux-arm64, macos) and updated transitive dependencies (vinyl 3.0.1, replace-ext 2.0.0, and new dependencies like streamx, teex, b4a)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

I seem to remember that I didn't land #24679 because it contained some code size regressions that we wanted addressed before we landed it.

Are the regressions in this change less serious than in #24679?

Is there any particular reason you want this upgrade? i.e. is there some feature of closure compiler you want?

/**
* @param {number=} maxWaitMilliseconds
*/
/** @suppress {duplicate, checkTypes} */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Merge this with the existing docstring above.

return True
if arg == '--externs' and i + 1 < len(args) and 'webgpu-externs.js' in args[i + 1]:
return True
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just keep this simple and do return any('webgpu-externs.js' in a for a in args).

Does it matter if webgpu-externs-fixes.js is included even when its not strictly needed?

I assume webgpu-externs.js is part of the emdawn project? If so we should probably file a bug there to get it fixed and leave commend here to remove this code once its fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants