-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
feat(js): Parse debug ids from scraped source files #1534
feat(js): Parse debug ids from scraped source files #1534
Conversation
Thank you very much for the contribution, I think this is a good idea and seems like a simple change. Tests would be appreciated! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1534 +/- ##
==========================================
- Coverage 76.09% 73.95% -2.15%
==========================================
Files 101 104 +3
Lines 15262 15870 +608
==========================================
+ Hits 11613 11736 +123
- Misses 3649 4134 +485 |
Do you have a test case demonstrating the improvement that you can add? |
I will add a test, I've just not had a chance yet! |
Test added! It has a source file which isn't included in the artifact bundle which causes it to be scraped. Once scraped, the debug id parsed and then used to lookup the sourcemap from the bundle. |
Very nice! Would you mind adding a changelog entry? |
I've been working on some bundler plugins that inject debug ids as per the TC39 proposal.
Since there's no cheap way to get debug ids from source files in the browser, this PR adds parsing of debug id from source files when they're scraped which can later be used to lookup the sourcemap.
The JavaScript SDKs can't rely on source files being publicly available and/or scraping working so we will still have to use polyfills and pass debug ids from the client. So this change doesn't improve things from the SDK perspective (no reduction in bundle or payload size), but I though symbolicator should probably still support this.
Let me know if this is worth adding and I'll try and add some tests!