V7: Breadcrumb refactor #650
Conversation
| @@ -1,19 +1,19 @@ | |||
| const { isoDate } = require('./lib/es-utils') | |||
|
|
|||
| class BugsnagBreadcrumb { | |||
| constructor (name = '[anonymous]', metaData = {}, type = 'manual', timestamp = isoDate()) { | |||
| constructor (message, metadata, type, timestamp = isoDate()) { | |||
There was a problem hiding this comment.
These default values have been removed in favour of the validation/default values being done in the client.leaveBreadcrumb() method rather than being split between there and here.
| // if no name and no metaData, usefulness of this crumb is questionable at best so discard | ||
| if (typeof name !== 'string' && !metaData) return | ||
| // if no message, discard | ||
| if (!message) return |
There was a problem hiding this comment.
The spec doesn't allow a breadcrumb without a message, so we are more strict about that now.
| @@ -124,19 +124,21 @@ class BugsnagClient { | |||
| return this._sessionDelegate.startSession(this) | |||
| } | |||
|
|
|||
| leaveBreadcrumb (name, metaData, type, timestamp) { | |||
| leaveBreadcrumb (message, metadata, type) { | |||
There was a problem hiding this comment.
timestamp is not in the public method signature according to the new spec
packages/core/client.js
Outdated
| const crumb = new BugsnagBreadcrumb(message, metadata, type) | ||
|
|
||
| // check the breadcrumb is the list of enabled types | ||
| if (!this.config.enabledBreadcrumbTypes || !includes(this.config.enabledBreadcrumbTypes, crumb.type)) return |
There was a problem hiding this comment.
enabledBreadcrumbTypes is guaranteed by config to be either null or an array of valid types.
There was a problem hiding this comment.
Is it worthwhile doing this check before instantiating a BugsnagBreadcrumb class instance?
There was a problem hiding this comment.
Ah yeah good point. We previously did it after because the default type="manual" was only populated after the instantiation, but now it's done inside this method we can do it earlier.
| severity: event.severity | ||
| }, 'error') | ||
| } | ||
| BugsnagClient.prototype.leaveBreadcrumb.call(this, event.errorClass, { |
There was a problem hiding this comment.
Node's notifier monkey-patches the leaveBreadcrumb() method to log a warning that breadcrumbs aren't supported.
Calling the method via the prototype ensures "internal" calls to leaveBreadcrumb() don't trigger the log. I can explain more if this doesn't make sense!
| public type: string; | ||
| public timestamp: string; | ||
| constructor(name: string, metaData?: object, type?: string, timestamp?: string); |
There was a problem hiding this comment.
Breadcrumb construction is not public, it can only be done via leaveBreadcrumb().
| const explicitlyDisabled = client.config.consoleBreadcrumbsEnabled === false | ||
| const implicitlyDisabled = (client.config.autoBreadcrumbs === false || isDev) && client.config.consoleBreadcrumbsEnabled !== true | ||
| if (explicitlyDisabled || implicitlyDisabled) return | ||
| if (!client.config.enabledBreadcrumbTypes || !includes(client.config.enabledBreadcrumbTypes, 'log') || isDev) return |
There was a problem hiding this comment.
This change has the known impact of meaning console log breadcrumbs can't be enabled in development.
packages/core/client.js
Outdated
| const crumb = new BugsnagBreadcrumb(message, metadata, type) | ||
|
|
||
| // check the breadcrumb is the list of enabled types | ||
| if (!this.config.enabledBreadcrumbTypes || !includes(this.config.enabledBreadcrumbTypes, crumb.type)) return |
There was a problem hiding this comment.
Is it worthwhile doing this check before instantiating a BugsnagBreadcrumb class instance?
packages/core/config.js
Outdated
| validate: value => value === null || (isArray(value) && reduce(BREADCRUMB_TYPES, (accum, maybeType) => { | ||
| if (accum === false) return accum | ||
| return includes(BREADCRUMB_TYPES, maybeType) | ||
| }, true)) |
There was a problem hiding this comment.
I think this validation logic is wrong. Consider the following failing test case:
describe('schema', () => {
...
describe('enabledBreadcrumbTypes', () => {
describe('validation', () => {
it('fails when a supplied value is not a valid breadcrumb type', () => {
const enabledBreadcrumbTypesValidator = config.schema.enabledBreadcrumbTypes.validate;
expect(enabledBreadcrumbTypesValidator(['UNKNOWN_BREADCRUMB_TYPE'])).toBe(false)
})
})
})
})
Gives Expected true to be false.
There was a problem hiding this comment.
Thanks for the failing test case 🙌
I added it to the codebase and fixed the bug (I was iterating over BREADCRUMB_TYPES rather than values).
enabledBreadcrumbTypesoptionThe
*BreadcrumbsEnabledplugin-specfic options have been removed and breadcrumbs are now controlled by generic list inenabledBreadcrumbTypes.breadcrumb.{ name -> message, metaData -> metadata }These fields have been renamed. Note that
metaData->metadatawill happen elsewhere in the codebase too but I limited the scope to just breadcrumbs for this PRleaveBreadcrumb()type signature to be more explicitThe
metadataobject is only allowed to be one level deep in breadcrumbs so the type signature now enforces that