-
Notifications
You must be signed in to change notification settings - Fork 38
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(core): Add experimental debug ID based source map upload to Rollup and Vite plugins #192
Conversation
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.
This needs tests, especially around the use strict
logic and determining source map paths.
|
||
// TODO: Find a more elaborate process to generate this. (Maybe with type checking and built-in minification) | ||
const DEBUG_ID_INJECTOR_SNIPPET = | ||
';!function(){try{var e="undefined"!=typeof window?window:"undefined"!=typeof global?global:"undefined"!=typeof self?self:{},n=(new Error).stack;n&&(e._sentryDebugIds=e._sentryDebugIds||{},e._sentryDebugIds[n]="__SENTRY_DEBUG_ID__",e._sentryDebugIdIdentifier="sdbid-__SENTRY_DEBUG_ID__")}catch(e){}}();'; |
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.
We can build this whenever we need to change it - same approach as we use here: getsentry/sentry#46780. I don't think we need to automate this, we can just manually run terser.
So everytime this snippet changes, we rebuild the minified JS. We can have this in follow up PR.
/** | ||
* Deterministically hashes a string and turns the hash into a uuid. | ||
*/ | ||
export function stringToUUID(str: string): string { |
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.
l: Can we have a test that enforces this will return a uuid - sanity check mostly that it behaves ok across Node versions.
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.
Added a small test: 4154b4a
* | ||
* Note: Currently only functional for Vite and Rollup. | ||
*/ | ||
debugIdUpload?: { |
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: Can we add the default values for include/ignore?
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 am not sure if a default for include makes sense, since you either define paths or you don't. Ignore makes sense though, so I added it: a7cf4e3
}); | ||
|
||
await writeSourceFilePromise; | ||
await writeSourceMapFilePromise; |
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.
l: Why double await here? If we want it to run at the same time, can we just do Promise.allSettled
?
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 think we want to throw here in case one of the promises fails so I went with Promise.all
: 025e4c1
} | ||
|
||
/** | ||
* Looks for a particular string pattern (`sdbid-[debug ID]`) in the bundle |
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.
Why did we chose sdbid
?
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.
Just wanted to use something short. It can be changed at any time since nothing depends on this. Do you think another pattern fits better?
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 would rather it be sentry
, but it's no big deal.
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 would rather it be sentry, but it's no big deal.
No! good point! :) befc007
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.
let's ship and try testing it out!
|
||
describe("stringToUUID", () => { | ||
test("should return a deterministic UUID", () => { | ||
expect(stringToUUID("Nothing personnel kid")).toBe("52c7a762-5ddf-49a7-6f16-6874a8cb2512"); |
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.
❤️
|
||
// For now we write both fields until we know what will become the standard - if ever. | ||
map["debug_id"] = debugId; | ||
map["debugId"] = debugId; |
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.
Ref #191
This PR adds an experimental option to the bundler plugins that will upload source files with debug IDs to Sentry.
In short, the following steps are executed:
//# debugId=...
comment at the bottom of the chunks and write the chunk to the temp folder.debugId
anddebug_id
field to the corresponding source maps if they are found and write them to the temp folder.