Skip to content
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: Add sourcemaps inject command #1469

Merged
merged 20 commits into from Feb 23, 2023
Merged

feat: Add sourcemaps inject command #1469

merged 20 commits into from Feb 23, 2023

Conversation

loewenheim
Copy link
Contributor

@loewenheim loewenheim commented Feb 10, 2023

This implements #1467.

It adds a new subcommand inject PATH to the sourcemaps command that works as follows:

For each .js file file.js in PATH,

  1. If file.js contains a //# sentryDebugId //# debugId comment, skip it (it has already been processed).
  2. If file.js does not contain a //# sourceMappingURL=<sourcemap_path> comment, skip it (without a sourcemap there's nothing to do).
  3. Check if sourcemap_path exists. If not, emit a warning and skip file.js.
  4. Check if the sourcemap at sourcemap_path contains a debug id under the debugID x_sentry_debug_iddebug_id key. If so, return it, otherwise insert a fresh debug id under that key and return that.
  5. Append a JS code snippet and a //# sentryDebugId=__SENTRY_DEBUG_ID__ comment to file.js, where __SENTRY_DEBUG_ID__ is replaced with the debug id returned in step 4.

This procedure differs slightly from the description in #1467: it checks first whether the sourcemap already contains a debug id instead of unconditionally generating a new one.

@loewenheim loewenheim self-assigned this Feb 10, 2023
pub fn make_command(command: Command) -> Command {
command
.about("Fixes up JavaScript source files and sourcemaps with debug ids.")
// TODO: What are these {n}{n}s? They show up verbatim in the help output.
Copy link
Contributor

Choose a reason for hiding this comment

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

They suppose to be terminal line-break specific to clap, but for some reason you say they stopped working? 🤔

https://github.com/clap-rs/clap/blob/ad5d67623a89415c7f5f7e1f7a47bf5abdfd0b6c/src/output/help_template.rs#L1018

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The placeholders have apparently been in the send_envelope-help test case since the beginning.

https://github.com/getsentry/sentry-cli/blame/master/tests/integration/_cases/send_envelope/send_envelope-help.trycmd

@kamilogorek
Copy link
Contributor

I'll review this next week, as I'm not even sure what we exactly want to achieve here tbh.

@mitsuhiko
Copy link
Member

I would vote to change x_sentry_debug_id to debug_id and sentryDebugId to just debugId. While right now we're clearly the main user here, I'm not sure adding a vendor prefix is doing anything in particular here. Not sure if anyone has strong opinions here.

@loewenheim
Copy link
Contributor Author

This now uses sourcemap-related functions that were added in symbolic 12.0.0. I couldn't find a good way to use discover_sourcemap_embedded_debug_id because we need to write to the sourcemap file anyway.

@mitsuhiko
Copy link
Member

mitsuhiko commented Feb 22, 2023

@loewenheim I think fixup_js_file should be a no-op if discover_sourcemap_embedded_debug_id returns Some. Otherwise that run is not idempotent and we add the snippet twice.

@loewenheim
Copy link
Contributor Author

But if the snippet was already added, the file should have a //# debugId comment and been skipped in the loop anyway. Or am I misunderstanding you?

@loewenheim
Copy link
Contributor Author

@lforst has convinced me that backing up the files before modifying them is the wrong thing to do, for two reasons:

  1. The files are easily restored from version control or regenerated if necessary.
  2. sentry-cli spamming .bak files into your directory would be very annoying.

@loewenheim loewenheim enabled auto-merge (squash) February 23, 2023 13:28
@loewenheim loewenheim merged commit 872bd77 into master Feb 23, 2023
@loewenheim loewenheim deleted the feat/sourcemap-inject branch February 23, 2023 13:41
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.

None yet

3 participants