feat: Add support for injecting Debug IDs#5712
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Imran-imtiaz48
left a comment
There was a problem hiding this comment.
The recent changes introduce new flexibility and configurability to the handling of sourcemaps and output validation, which is a welcome improvement. Here are some thoughts and observations on the adjustments:
-
sourcemapFileNames Enhancements:
- The added flexibility in defining
sourcemapFileNameseither as astringor a function ((chunkInfo: PreRenderedChunk) => string) is a good approach. This allows for more customized and dynamic sourcemap file naming. Consider documenting examples of how this can be used to help other developers understand the potential benefits of this feature.
- The added flexibility in defining
-
sourcemapIgnoreList Option:
- The introduction of
sourcemapIgnoreListis great for situations where you want to ignore specific files from sourcemapping. If possible, it might be helpful to expand on howSourcemapIgnoreListOptionworks. Are there specific cases or examples that could be highlighted to guide its usage?
- The introduction of
-
sourcemapPathTransform Functionality:
- Including
sourcemapPathTransformallows for powerful path transformations, which will be especially helpful for complex build environments. Clarifying its usage and providing a brief note on how it transforms paths could prevent confusion.
- Including
-
Debugging with sourcemapDebugIds:
- The
sourcemapDebugIdsflag is an interesting addition. It would be beneficial to explain the scenarios where enabling this would be most helpful. This could enhance debugging processes, but highlighting its impact on performance or build complexity would be useful.
- The
-
strict and systemNullSetters:
- Keeping the
strictandsystemNullSettersoptions ensures robust validation and system behavior. However, it might be worthwhile to indicate any potential performance impacts or if there are edge cases that developers should be aware of when using these settings.
- Keeping the
-
Overall Validation:
- The
validateflag is crucial for maintaining the integrity of the output. It’s always reassuring to see validation options prominently available to avoid runtime issues.
- The
Recommendations:
- Documentation: Consider enhancing the documentation with examples and scenarios for these settings. This will make the new features more approachable and usable for a broader range of developers.
- Performance Considerations: If any of these settings significantly impact build or runtime performance, detailing this in the documentation or with warning notes could be helpful.
It's to help match up sourcemaps to their respective source files when de-minifying production stack traces. Without debug ids all you have to find the correct sourcemap is the filename or perhaps a Which release version are you currently using? What if your app is made up from many different components which are all released individually? Assuming you even have a If both source and sourcemap contain a matching unique id, it's very easy to match them up and the above issues disappear. This is not a new idea. Native builds and debug symbols have been linked by unique ids for decades! Sentry have a blog post going into the above in more detail.
Rolldown always computes hashes for every source file so injecting an id derived from that hash adds minimal overhead. For Rollup, I found that hashes were only calculated when they are required so my PR add an extra full hashing of every source file when |
lukastaegert
left a comment
There was a problem hiding this comment.
This looks actually good to me, thank you so much for taking the time!
What is missing to merge this would be updating documentation. I can support you, but you might be able to write a better short description of the option. These are the places:
docs/configuration-options/index.md: I would add an entry toAdvanced functionality. As a boolean, it would be similar to e.g.output.inlineDynamicImports. Note that options are sorted alphabetically here within each section.- The option should be added to the list in
docs/javascript-api/index.mdin the### outputOptions objectsection. Again, note the alphabetical sorting. - Then it should be added to the
## Command line flagssection indocs/command-line-interface/index.md. Again, note the sorting. Each line has a short description, but it should observe the 80 character limit for each line. - Last, this section is copied over into
cli/help.md, which is the source for the command line help.
Thank you and let me know if you need help.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5712 +/- ##
==========================================
- Coverage 99.02% 99.00% -0.02%
==========================================
Files 258 258
Lines 8062 8069 +7
Branches 1359 1360 +1
==========================================
+ Hits 7983 7989 +6
- Misses 52 53 +1
Partials 27 27 ☔ View full report in Codecov by Sentry. |
|
I added the docs so let me know if I got it all correct! From the looks of the coverage failure, I need to add a test for debug ids + files with placeholders? |
Yes. To do that, you could set Documentation looks good, but I wonder if there might be a link to some external documentation we want to add to explain the feature better to the user, see also the comment by @Imran-imtiaz48 |
|
I think this PR is now ready.
|
|
This PR has been released as part of [email protected]. You can test it via |
This PR contains:
Are tests included?
Breaking Changes?
Description
Adds support for injecting debug IDs into source files and sourcemaps as per the TC39 proposal.