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

fix: Handle the formatting of array arguments #1139

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dentuzhik
Copy link

@dentuzhik dentuzhik commented Apr 24, 2024

Description of the change

We are using rollbar.js in our application, and because of internal monkey-patching of console.warn some instances of invariants triggered by apollo-client crash rollbar and our application as a result.

When apollo-client logs those invariants via console.warn it has some of the arguments specified as array of objects, which get through the formatArgsAsString inside rollbar, which does not really handle arrays anyhow and crashes.

This change adds graceful handling of the arrays via a simple recursive call, trimming the array to the first 10 items not to cause any significant overhead on processing.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Maintenance
  • New release

Related issues

Shortcut stories and GitHub issues (delete irrelevant)

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers assigned
  • Issue from task tracker has a link to this pull request
  • Changes have been reviewed by at least one other engineer

@dentuzhik
Copy link
Author

@waltjones @mudetroit apologies for tagging directly, I would appreciate if we can review this and draft a new patch release.

This issue causing a crash for us in production, the backup plan so far is to go through publishing an internal fork via private registry package, but we would still want to switch back to public version.

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.

1 participant