Skip to content
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

Recursive metadata is now handled in react native #1643

Merged
merged 1 commit into from
Jan 19, 2022

Conversation

lemnik
Copy link
Contributor

@lemnik lemnik commented Jan 11, 2022

Goal

Make it safe to specify complex and recursive objects as metadata for breadcrumbs and events in react native applications.

Design

Added a new makeSafe function to the delivery-react-native which behaves similarly to the safe-json-stringify module but specifically for the react native JS bridge, which does not invoke toJSON functions.

Testing

New unit tests to cover the makeSafe function, and adjustments to the existing Mazerunner tests to ensure circular references are correctly flattened and don't cause a crash.

@lemnik lemnik force-pushed the PLAT-7711/rn-recursive-metadata branch from 3363f11 to b6afa26 Compare January 11, 2022 14:25
@github-actions
Copy link

github-actions bot commented Jan 11, 2022

@bugsnag/browser bundle size diff

Minified Minfied + Gzipped
Before 41.58 kB 12.80 kB
After 41.58 kB 12.80 kB
± No change No change

code coverage diff

Ok File Lines Branches Functions Statements
/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/delivery-react-native/make-safe.js 89.47%
(+89.47%)
84.62%
(+84.62%)
83.33%
(+83.33%)
87.5%
(+87.5%)

Total:

Lines Branches Functions Statements
81.65%(+0.07%) 71.52%(+0.16%) 82.67%(+0%) 80.71%(+0.06%)

Generated by 🚫 dangerJS against b9e6abc

@lemnik lemnik marked this pull request as ready for review January 11, 2022 14:42
@lemnik lemnik force-pushed the PLAT-7711/rn-recursive-metadata branch from b6afa26 to 2500451 Compare January 12, 2022 17:28
}

// handle arrays, and all iterable non-array types (such as Set)
if (isArray(obj) || obj[Symbol.iterator]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all versions of RN support Symbol?

I can't really find anything very helpful, but there's this error from not having a Symbol global on SO which may mean we need to be careful about using Symbol

Something like this might be safer:

Suggested change
if (isArray(obj) || obj[Symbol.iterator]) {
if (isArray(obj) || (Symbol && obj[Symbol.iterator])) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Symbol is available on all the RN versions we support, but I'm happy to addd the secondary check for safety sake.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's supported everywhere then it's probably fine as-is, I just couldn't find anything definitive in a quick search

Comment on lines +10 to +11
// ensure that circular references are safely handled
metaData.circle = metaData
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add this to the expected breadcrumb?

import makeSafe from '../make-safe'

describe('delivery: react native makeSafe', () => {
it('leaves simple types intact', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add more types to this test? e.g. null & undefined (as they are special cased), Symbol, maybe a Map or other non-array iterable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants