New: Log `createFlash` errors in Sentry
createFlash
errors in Sentry
New Pattern Proposal: Log We currently use createFlash
pretty extensively through the codebase to display feedback to a user after some kind of action, including reporting errors. We have now also introduced Sentry js and are starting to make use of Sentry.captureException
to capture errors.
Perhaps instead of sprinking calls to Sentry.captureException
across the codebase, now might be a good opportunity to update the interface of createFlash
to also handle logging to sentry.
Advantages of new pattern
- We capture errors that are potentially getting missed
- We can gather some context from the user when an error occurs
- We have the building blocks already
createFlash
+Sentry.captureException
Disadvantages of new pattern
- Who actions errors coming from Sentry?
- Do we actually want to capture this all in Sentry?
- Probably something else that will need to be mocked in tests
- Possibly some data sanitation / privacy issues to consider
Proposals
The createFlash
function currently takes quite a few parameters, so there are multiple ways we could approach this
const createFlash = function createFlash(
message,
type = FLASH_TYPES.ALERT,
parent = document,
actionConfig = null,
fadeTransition = true,
addBodyClass = false,
)
What is the impact on our existing codebase?
A naive search across the code base returns 200+ calls to createFlash
across 100+ files
⛔ ️
Proposal 1: Nothing Nah, let's just sprinkle Sentry.captureException
and keep things the way they are
🎁
Proposal 2: Wrap createFlash arguments in a object We could move the arguments into an object and then add additional properties for logging to sentry
// Before
const createFlash = function createFlash(
message,
type = FLASH_TYPES.ALERT,
parent = document,
actionConfig = null,
fadeTransition = true,
addBodyClass = false,
){
...
}
// After
const createFlash = function createFlash({
message,
type = FLASH_TYPES.ALERT,
parent = document,
actionConfig = null,
fadeTransition = true,
addBodyClass = false,
captureError = false //
error = null // the error that was thrown
}) {
...
if (captureError){
Sentry.captureException(error)
}
}
This could be a nice approach, keeping it fairly clear what is happening, but would mean we would need to touch a lot of files.
🆕
Proposal 3: Expose a new function export const reportError = ({ message, error, sendToSentry = true, type = FLASH_TYPES.ALERT }) {
// send to sentry
// create flash
}
In this proposal we would provide a new function that both sends the error to sentry and creates a flash message, similar to proposal 2, but this approach would not affect any existing uses of createFlash
allowing a more gradual migration, it does mean another thing to remember / think about though.
🚢
Proposal 4: Go all in (kinda) We could add a 7th argument to createFlash (the error
raised) and always send the error to Sentry if present. Its not elegant, but this approach would also allow gradual migration without adding an additional function, or modifying existing code.
Reference implementation
This came about from a discussion about logging errors in analytics