-
Notifications
You must be signed in to change notification settings - Fork 255
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
Fix crashes when enabledBreadcrumbTypes
is null
#1467
Fix crashes when enabledBreadcrumbTypes
is null
#1467
Conversation
enabledBreadcrumbTypes
is nullenabledBreadcrumbTypes
is null
|
Minified | Minfied + Gzipped | |
---|---|---|
Before | 41.31 kB |
12.72 kB |
After | 41.33 kB |
12.72 kB |
± | +12 bytes |
+6 bytes |
code coverage diff
Ok | File | Lines | Branches | Functions | Statements |
---|---|---|---|---|---|
✅ | /home/runner/work/bugsnag-js/bugsnag-js/packages/core/client.js | 99.34% (+0%) |
97.62% (+0.06%) |
100% (+0%) |
98.8% (+0.01%) |
🔴 | /home/runner/work/bugsnag-js/bugsnag-js/packages/electron/src/client/renderer.js | 34.38% (-1.1%) |
0% (+0%) |
0% (+0%) |
30.56% (-0.87%) |
✅ | /home/runner/work/bugsnag-js/bugsnag-js/packages/plugin-electron-ipc/bugsnag-ipc-main.js | 82.54% (+0%) |
75% (+1.92%) |
89.47% (+0%) |
81.43% (+0%) |
✅ | /home/runner/work/bugsnag-js/bugsnag-js/packages/plugin-electron-net-breadcrumbs/net-breadcrumbs.js | 100% (+0%) |
90% (+2.5%) |
100% (+0%) |
100% (+0%) |
🔴 | /home/runner/work/bugsnag-js/bugsnag-js/packages/react-native/src/notifier.js | 72.15% (+0.36%) |
61.76% (-0.74%) |
61.54% (+0%) |
71.26% (+0.33%) |
Total:
Lines | Branches | Functions | Statements |
---|---|---|---|
82.26%(-0.01%) | 71.55%(+0.07%) | 83.47%(+0%) | 81.2%(-0.01%) |
6884c1f
to
5a27300
Compare
caa707a
to
0943969
Compare
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.
Is it possible to either:
- Coerce
enabledBreadcrumbTypes
to always be an array during config validation (where null ->all
)? or - Add a helper on config to handle the null check? Most of the validation checks now are of the form
if (config.breadcrumbTypes !== null && config.breadcrumbTypes.contain(type))
where maybe it could be something likeif (config.isBreadcrumbTypeEnabled(type))
Trying to think of a way to prevent this kind of issue going forward since every future plugin/consumer of this config option basically needs to have this check. Stronger type validation on our part? Additional linting?
0943969
to
341d6c7
Compare
I avoided doing anything extra in core to try to avoid increasing the browser bundle size, but it turns out that a helper method actually saves space because it removes duplication from plugins I've opened #1468 for this as it affects files in this PR, #1466 and files that weren't touched by either PR |
Goal
Following on from #1466, this PR fixes crashes in various plugins that assumed
enabledBreadcrumbTypes
must always be an array. They now allow for the case where it may be null and handle it accordinglyTesting
Existing tests & new unit tests. Manually tested in browser,
electronandreact nativeprojects