Skip to content

Comments

feat: Add support for injecting Debug IDs#5712

Merged
lukastaegert merged 5 commits intorollup:masterfrom
timfish:feat/debug-ids
Nov 9, 2024
Merged

feat: Add support for injecting Debug IDs#5712
lukastaegert merged 5 commits intorollup:masterfrom
timfish:feat/debug-ids

Conversation

@timfish
Copy link
Contributor

@timfish timfish commented Nov 1, 2024

This PR contains:

  • feature

Are tests included?

  • yes (bugfixes and features will not be merged without tests)

Breaking Changes?

  • no

Description

Adds support for injecting debug IDs into source files and sourcemaps as per the TC39 proposal.

  • Sentry users have been relying on debug ids in production for a number of years. Sentry process hundreds of millions of files per month containing debug ids!
  • A Github search suggests debug ids are also used by Backtrace and Expo
  • Bun added debug id injection to it's bundler when they added sourcemap support
  • Rolldown just merged debug id support

@vercel
Copy link

vercel bot commented Nov 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rollup ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 6, 2024 9:26am

Copy link

@Imran-imtiaz48 Imran-imtiaz48 left a comment

Choose a reason for hiding this comment

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

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:

  1. sourcemapFileNames Enhancements:

    • The added flexibility in defining sourcemapFileNames either as a string or 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.
  2. sourcemapIgnoreList Option:

    • The introduction of sourcemapIgnoreList is great for situations where you want to ignore specific files from sourcemapping. If possible, it might be helpful to expand on how SourcemapIgnoreListOption works. Are there specific cases or examples that could be highlighted to guide its usage?
  3. sourcemapPathTransform Functionality:

    • Including sourcemapPathTransform allows 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.
  4. Debugging with sourcemapDebugIds:

    • The sourcemapDebugIds flag 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.
  5. strict and systemNullSetters:

    • Keeping the strict and systemNullSetters options 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.
  6. Overall Validation:

    • The validate flag is crucial for maintaining the integrity of the output. It’s always reassuring to see validation options prominently available to avoid runtime issues.

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.

@timfish
Copy link
Contributor Author

timfish commented Nov 1, 2024

It would be beneficial to explain the scenarios where enabling this would be most 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 sourceMappingURL. If your app has release versions you can use that to help differentiate but combined with the optional sourceMappingURL this has many pitfalls.

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 sourceMappingURL in the source, is it correct? Getting the correct sourceMappingURL emitted into a sourcemaps has plagued devs for years.

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.

but highlighting its impact on performance or build complexity would be useful.

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 sourcemapDebugIds is enabled.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

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 to Advanced 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.md in the ### outputOptions object section. Again, note the alphabetical sorting.
  • Then it should be added to the ## Command line flags section in docs/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
Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 99.00%. Comparing base (cdf34ab) to head (ffb30d0).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/utils/renderChunks.ts 83.33% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@timfish
Copy link
Contributor Author

timfish commented Nov 4, 2024

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?

@lukastaegert
Copy link
Member

lukastaegert commented Nov 4, 2024

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 options.output.entryFileNames to something like [hash].js in your test _config.js file. Basically it means that file has a hash in its file name.

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

@timfish
Copy link
Contributor Author

timfish commented Nov 6, 2024

I think this PR is now ready.

  • Added another test to give full test coverage
  • Added a link to the TC39 proposal
  • Fixed the missing arg in help.md

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Looks really good!

@lukastaegert lukastaegert disabled auto-merge November 9, 2024 06:41
@lukastaegert lukastaegert merged commit d0935a8 into rollup:master Nov 9, 2024
@timfish timfish deleted the feat/debug-ids branch November 9, 2024 08:38
@github-actions
Copy link

github-actions bot commented Nov 9, 2024

This PR has been released as part of [email protected]. You can test it via npm install rollup.

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.

4 participants