Handle recursive metadata in ReactNative #1673
Conversation
…t to native notifiers
… with the native notifiers
# Conflicts: # packages/delivery-react-native/test/make-safe.test.ts # packages/plugin-react-native-client-sync/package.json
|
| Minified | Minfied + Gzipped | |
|---|---|---|
| Before | 42.85 kB |
13.07 kB |
| After | 42.85 kB |
13.07 kB |
| ± | No change | No change |
code coverage diff
| Ok | File | Lines | Branches | Functions | Statements |
|---|---|---|---|---|---|
| ✅ | /home/runner/work/bugsnag-js/bugsnag-js/packages/core/lib/derecursify.js | 97.37% (+97.37%) |
96.15% (+96.15%) |
100% (+100%) |
97.5% (+97.5%) |
| ✅ | /home/runner/work/bugsnag-js/bugsnag-js/packages/delivery-react-native/delivery.js | 72.73% (+2.73%) |
42.86% (+0%) |
60% (+0%) |
75% (+2.27%) |
| ✅ | /home/runner/work/bugsnag-js/bugsnag-js/packages/plugin-react-native-client-sync/client-sync.js | 85.71% (+0.2%) |
47.06% (+0%) |
100% (+0%) |
86.11% (+0.19%) |
Total:
| Lines | Branches | Functions | Statements |
|---|---|---|---|
| 82.03%(+0.14%) | 71.73%(+0.29%) | 83.17%(+0.09%) | 81.1%(+0.14%) |
lemnik
force-pushed
the
PLAT-7711/rn-recursive-metadata
branch
from
bccfe5b
to
c647b22
Compare
| @@ -1,4 +1,5 @@ | |||
| const { DeviceEventEmitter, NativeEventEmitter, NativeModules, Platform } = require('react-native') | |||
| const makeSafe = require('@bugsnag/delivery-react-native/make-safe') | |||
There was a problem hiding this comment.
I don't think it makes sense for this to import something from delivery-react-native
Should make-safe be a new plugin? We could put it in core, but that would increase the browser bundle size, so maybe it could go in @bugsnag/react-native?
There was a problem hiding this comment.
It'd be good to give it a better name now it's being used in multiple places as well; maybe something like "resolve recursive references" (or something less alliterative!)
There was a problem hiding this comment.
I mostly agree with moving it out of delivery, but moving it into @bugsnag/react-native leads us to a circular dependency. Given how tied to the react-native bridge this logic is, it should definitely be kept away from core.
Also agree on changing the name, maybe bridgeSafe?
Alternatively it needs to become more generic and move completely to its own module?
There was a problem hiding this comment.
Given how tied to the react-native bridge this logic is…
I'm not sure it is tied to the react-native bridge; it just resolves recursive references in any JS value, it isn't RN specific AFAICT
There was a problem hiding this comment.
In the abstract, yes. On the other hand it also converts Date objects to strings, changes how Error objects are represented, etc. while those are probably useful behaviours everywhere, it's been build with the react native bridge in mind and is baked in logic.
…e meta-data in React Native applications This reverts commit 99a0cb4
lemnik
force-pushed
the
PLAT-7711/rn-recursive-metadata
branch
from
c647b22
to
69bdd2a
Compare
Goal
Re-introduce the changes made in #1643 with fixes to the tests, and an additional
makeSafecall inplugin-react-native-client-syncto ensure that metadata included in Breadcrumbs is also safe to cross the React Native JS Bridge.Make it safe to specify complex and recursive objects as metadata for breadcrumbs and events in react native applications.
Design
Added a new
makeSafefunction to thedelivery-react-nativewhich behaves similarly to thesafe-json-stringifymodule but specifically for the react native JS bridge, which does not invoketoJSONfunctions.Testing
New unit tests to cover the
makeSafefunction, and adjustments to the existing Mazerunner tests to ensure circular references are correctly flattened and don't cause a crash.