Skip to content

feat(sourcemaps): Support injecting indexed sourcemaps #2470

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 30, 2025
Merged

feat(sourcemaps): Support injecting indexed sourcemaps #2470

merged 2 commits into from
Apr 30, 2025

Conversation

@szokeasaurusrex
Copy link
Member

Closes #2463
Closes #2466

szokeasaurusrex
szokeasaurusrex commented Apr 25, 2025
View reviewed changes
Cargo.toml Outdated
Comment on lines 111 to 112
[patch.crates-io]
sourcemap = { git = "https://github.com/getsentry/rust-sourcemap.git", rev = "e8dff4fbb74f346d0f13a2f547cedf4b5fb4e83f" }
Copy link
Member Author

Choose a reason for hiding this comment

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

Allows us to use stuff from getsentry/rust-sourcemap#119 and getsentry/rust-sourcemap#118. We will need to release these PRs upstream before this PR can be merged

loewenheim
loewenheim reviewed Apr 25, 2025
View reviewed changes
Copy link
Contributor

@loewenheim loewenheim left a comment

Choose a reason for hiding this comment

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

The injection procedure itself looks good. See comments about the matching logic though.

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/use-injection branch from 75bc7c4 to e88d1b1 Compare April 28, 2025 13:07
szokeasaurusrex
szokeasaurusrex commented Apr 28, 2025
View reviewed changes
Copy link
Member Author

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

@loewenheim I have addressed your comments. Plan to also add some integration tests – in the meantime, please let me know if you have other feedback

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/use-injection branch 3 times, most recently from 853981c to 4ef911c Compare April 28, 2025 13:38
@szokeasaurusrex szokeasaurusrex mentioned this pull request Apr 28, 2025
Merged
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/use-injection branch from 4ef911c to aa9493a Compare April 29, 2025 09:54
szokeasaurusrex
szokeasaurusrex commented Apr 29, 2025
View reviewed changes
Copy link
Member Author

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

@loewenheim, I have added an integration test. Do you think any more changes need to be made to this PR? If not, I'll merge getsentry/rust-sourcemap#119 and getsentry/rust-sourcemap#118, release those, update this PR to depend on the new version, and mark it as ready for review

loewenheim
loewenheim approved these changes Apr 29, 2025
View reviewed changes
Copy link
Contributor

@loewenheim loewenheim left a comment

Choose a reason for hiding this comment

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

Looks good, apart from a leftover comment.

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/use-injection branch 3 times, most recently from f8c46b3 to 40af3a9 Compare April 29, 2025 13:44
@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review April 29, 2025 13:44
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/use-injection branch from 40af3a9 to 83d21a9 Compare April 30, 2025 11:30
szokeasaurusrex
szokeasaurusrex commented Apr 30, 2025
View reviewed changes
Copy link
Member Author

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

Postponing merge until #2482 is addressed – I want to make sure that the injection behavior for non-indexed sourcemaps is unchanged

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/use-injection branch from 83d21a9 to bd121fc Compare April 30, 2025 11:48
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/use-injection branch from bd121fc to 01fc527 Compare April 30, 2025 13:00
@szokeasaurusrex szokeasaurusrex merged commit 01fc527 into master Apr 30, 2025
17 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/use-injection branch April 30, 2025 14:37
szokeasaurusrex added a commit that referenced this pull request May 8, 2025
@szokeasaurusrex
ref: Rename fixup_js_file_end (#2475)
47e426d 
Depends on #2470
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@loewenheim loewenheim loewenheim approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enable re-injecting "use [...]" before Debug ID code snippet Index Sourcemaps not supported in sourcemaps inject

2 participants

@szokeasaurusrex @loewenheim