-
-
Notifications
You must be signed in to change notification settings - Fork 240
feat(js-lib): Add sourceMaps.inject() for injecting debug IDs
#3088
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
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
lib/releases/index.ts
Outdated
| * @param options Options to configure the source map upload. | ||
| * @returns A promise that resolves when the upload has completed successfully. | ||
| */ | ||
| // TODO: Move `uploadSourceMaps()` from Releases to SourceMaps class as `.upload()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, but I think we can leave this command in place to avoid breaking anyone once we move things over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the comment a bit so we know of this in the future
JPeer264
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| * @param options Options to configure the source map upload. | ||
| * @returns A promise that resolves when the upload has completed successfully. | ||
| */ | ||
| // TODO: Add `uploadSourceMaps()` to SourceMaps class as `.upload()` (keep it here too for backward compatibility) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@s1gr1d did you mean to address this TODO in the current changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that would be another PR. It's an unrelated change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: Got it, is there an issue for this already? If not, can you please make one? 🙏
l: I would suggest we delete the // TODO comment as the issue should suffice.
| // TODO: Add `uploadSourceMaps()` to SourceMaps class as `.upload()` (keep it here too for backward compatibility) |
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, please see most recent comment prior to merge
| * @param options Options to configure the source map upload. | ||
| * @returns A promise that resolves when the upload has completed successfully. | ||
| */ | ||
| // TODO: Add `uploadSourceMaps()` to SourceMaps class as `.upload()` (keep it here too for backward compatibility) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: Got it, is there an issue for this already? If not, can you please make one? 🙏
l: I would suggest we delete the // TODO comment as the issue should suffice.
| // TODO: Add `uploadSourceMaps()` to SourceMaps class as `.upload()` (keep it here too for backward compatibility) |
Description
The JS Bundler Plugins repo uses the CLI instance in the Build Plugin Manager. However, injecting debug IDs is handled with a "raw"
.execute(). This PR adds this directly to the CLI instance under the namespacesourceMaps. Uploading source maps can be handled through this namespace as well in the future.related PR: getsentry/sentry-javascript-bundler-plugins#836