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

adding setContextsInterface #809

Closed
wants to merge 1 commit into from
Closed

Conversation

orlando
Copy link

@orlando orlando commented Dec 27, 2016

Context

This PR adds setContextsInterface method, which allows using the Contexts Interface.

A use case is when using this library in React Native. You can send device and os information along with the error.

Notes

  • The user has to know about how the Contexts Interface works. Given that not every key is valid.
  • We don't send this data unless the user uses the method (or access the _globalContext private variable directly)

Media

Before

image

After

image

@dcramer
Copy link
Member

dcramer commented Dec 28, 2016

@getsentry/javascript we should talk in person, but I'm +1 on getting an API in for this, but likely under a more friendly interface.

I put 10 seconds of thought into this, but e.g.

Raven.setContext('foo', {})
  • We'd probably also want to call it set vs add since it'd overwrite that keyname
  • We probably don't need interface in the function call

@orlando
Copy link
Author

orlando commented Dec 28, 2016

Great @dcramer,

I like that interface. One thing to keep in mind is how we handle calling this method with no arguments or with the second argument missing.

But let me know what changes we should do to get this merge.

Thanks!

@orlando
Copy link
Author

orlando commented Feb 6, 2017

hey @dcramer any update on this?

@dcramer
Copy link
Member

dcramer commented Feb 6, 2017

@getsentry/js

@dcramer
Copy link
Member

dcramer commented Feb 6, 2017

Welp maybe @getsentry/javascript

@benvinegar
Copy link
Contributor

benvinegar commented Feb 9, 2017

@orlando – a couple things:

  1. There is a new React Native plugin in the works that will provide this information automatically.
  2. I find the naming of "contexts" interface confusing. There are already setUserContext, setExtraContext and setTagsContext, not to mention getContext and clearContext. I am not fond of bloating the API further with setContextsInterface, which further strains the meaning of "context" and adds a new term, "interface", which has not been exposed in the naming scheme before.

Option 2 makes me want to just punt until the new RN client is out.

@orlando
Copy link
Author

orlando commented Feb 9, 2017

@benvinegar great!. Thanks for the info. Can't wait to kill my fork.

Thanks again!

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