Conversation
650f0e3 to
1fd9e45
Compare
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR removes the SetupEmscripten C++ code path and instead imports setupEmscripten.ts directly as an internal module, simplifying the Emscripten initialization pipeline.
- [HIGH] Debug
print()statement left inhelpers.bzl— this will produce output on every Bazel load of this file, polluting build logs. - [LOW]
python_stdlib.zip.d.tsis now a dead file — thedeclare module 'pyodide-internal:generated/python_stdlib.zip'was removed but the file remains with bare top-level declarations that nothing references. Consider deleting it entirely sincemodules.d.tsalready declares the newpyodideRuntime-internal:python_stdlib.zipmodule.
This review was generated by an AI assistant and may contain inaccuracies.
|
Review posted on PR #6658 with two findings:
The rest of the PR looks clean: all |
Merging this PR will not alter performance
Comparing Footnotes
|
|
Goodbye old friend - you will not be missed |
ryanking13
left a comment
There was a problem hiding this comment.
I don't have the exact context of why it was existed in C++ originally, but the change looks good overall.
dom96
left a comment
There was a problem hiding this comment.
I wonder if there are other things we can remove now that SetupEmscripten is gone.
3341241 to
421fb9d
Compare
421fb9d to
6071578
Compare
Instead import `setupEmscripten.ts` directly. As requested by jasnell because SetupEmscripten does not work with new module registry.
6071578 to
98ad560
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6658 +/- ##
==========================================
- Coverage 66.57% 66.56% -0.02%
==========================================
Files 403 402 -1
Lines 115958 115889 -69
Branches 19416 19404 -12
==========================================
- Hits 77198 77137 -61
+ Misses 27182 27180 -2
+ Partials 11578 11572 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Instead import
setupEmscripten.tsdirectly. As requested by @jasnell because SetupEmscripten does not work with new module registry.