[NO QA][Sentry] Adding module names to web#84422
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| // Build a moduleId → file path map so numeric production IDs are readable in Sentry. | ||
| // Skip node_modules to keep the emitted JSON small. | ||
| const names: Record<string, string> = {}; | ||
| for (const module of compilation.modules) { |
There was a problem hiding this comment.
Hmmm. This is a going to add a non-trivial amount of startup JS for every user in production, as we're effectively shipping a moduleId -> path manifest for every first-party webpack module in the app inside the runtime chunk.
Could we rework this to avoid shipping the full map at startup? cc: @roryabraham
There was a problem hiding this comment.
I wonder if we can solve this at build time instead of runtime (i.e: avoid mangling the module names in the first place)
There was a problem hiding this comment.
We will do the JSON parsing for every user on startup, it might impact our app startup metrics negatively.
Eli do you think we are able to do it on build time rather than runtime?
There was a problem hiding this comment.
I will check it and let you know
There was a problem hiding this comment.
ok I changed it and now it should execute in build time, there one trade off which it need to execute 1 async action (fetch) but I think its minor effect. what do you think ? @roryabraham @inimaga
The map is now built at webpack compile time and written to a static file module-names.json in dist/. It never touches the runtime chunk. The app fetches it lazily inside requestAnimationFrame
There was a problem hiding this comment.
I personally think a lazy fetch of a json file is probably a small price to pay for this visibility.
nit: why requestAnimationFrame instead of requestIdleCallback? The latter seems more idomatic for tasks unrelated to UI.
There was a problem hiding this comment.
Sure I changed it and pushed other minor change due to an exception I had sometimes of Multiple assets emit different content to the same filename module-names.json. now it seems working good.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required on this one.
|
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
|
I might refactor anyway so ill check after well decide what to do |
Julesssss
left a comment
There was a problem hiding this comment.
@elirangoshen could you please share some before/after metrics for reference
so for the web span ManualAppStartup was better after the change , naybe due ti other chnages in main branch : |
Julesssss
left a comment
There was a problem hiding this comment.
Looking good, one minor comment
|
Sorry for delay, was ooo |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚧 @Julesssss has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.43-3 🚀
|
Explanation of Change
In production webpack builds, module IDs are numeric hashes (e.g. 535, 23042) instead of file paths. The Sentry breadcrumb for slow module init times was reporting these opaque numbers, making the data unusable
for identifying performance bottlenecks.
The changes:
ModuleInitTimingPlugin.ts— at build time, the plugin now iterates all webpack compilation modules and emits aself.__moduleNames = { "535": "./src/styles/index.ts", ... }runtime object mapping each numericID to its relative source path. node_modules are excluded to keep the emitted JSON small.
index.web.ts— now reads __moduleNames (same typeof guard pattern used for__moduleInitTimes) and passes it toreportModuleInitTimes, which already uses it as a fallback:names?.[id] ?? id.The native side (Metro) already had this via
moduleInitPolyfill.ts+__moduleNames.This brings web to parity.Fixed Issues
$#82975
Tests
Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionsrc/languages/*files and using the translation methodScrollViewcomponentmainbranch was merged into this PR after a review, I tested again and verified the outcomeScreenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari