Skip to content

Angular and reactNative data callback should return updated data. #891

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

Merged
merged 2 commits into from
May 15, 2017

Conversation

graingert
Copy link
Contributor

No description provided.

@graingert graingert force-pushed the patch-3 branch 2 times, most recently from 091671c to 949d1b8 Compare March 15, 2017 12:53
@graingert
Copy link
Contributor Author

@benvinegar I don't think this stops adding any of the proposed changes from #803 but it fixes a real problem that exists now.

@graingert graingert changed the title Angular data callback should return updated data. Angular and reactNative data callback should return updated data. Mar 15, 2017
@graingert graingert force-pushed the patch-3 branch 3 times, most recently from 70c0ef0 to 19fe83d Compare March 22, 2017 12:54
@graingert
Copy link
Contributor Author

@benvinegar ping ^

@benvinegar
Copy link
Contributor

The API is really bloated and I'd prefer not to add wrapDataCallback (because then it follows that we'd also need wrapShouldSendCallback and the others).

Can this be done without that?

@graingert
Copy link
Contributor Author

graingert commented Mar 23, 2017

@benvinegar I did add those, and it's not that much extra code, in exchange I re-factored the duplicate setXCallback APIs

the setXCallback APIs should be deprecated.

@benvinegar
Copy link
Contributor

the setXCallback APIs should be deprecated.

Unfortunately, we have a goal with this project which is to change the APIs as infrequently as possible. And if we're going to do it, it better really be worth it. In this case, I'm not ready to change these APIs until we commit to moving towards the middleware style advocated in #803.

It's also not clear to me what problem this PR is solving, because there are no tests or example failure cases.

@graingert
Copy link
Contributor Author

@benvinegar the issue is, setDataCallback supports two ways of changing the data sent:

  1. The Good way: return a new object with the new data
  2. The Naughty way: alter the internals of the object 'mutation' 🤮

however the angular and react-native clobber method 1 of the original functions by not returning the data through the stack of functions.

@graingert
Copy link
Contributor Author

@benvinegar happy to rename wrapXCallback methods to _wrapXCallback methods

@benvinegar
Copy link
Contributor

Okay, after a long conversation on IRC, I think this closes up some holes in the callback system.

Can you maybe add a cursory set of utils tests for wrappedCallback, and I'll accept.

@graingert graingert force-pushed the patch-3 branch 4 times, most recently from 489c77a to 3797401 Compare March 23, 2017 02:01
@graingert
Copy link
Contributor Author

@benvinegar any update on this?

2 similar comments
@graingert
Copy link
Contributor Author

@benvinegar any update on this?

@graingert
Copy link
Contributor Author

@benvinegar any update on this?

@benvinegar benvinegar merged commit 9402557 into getsentry:master May 15, 2017
@benvinegar
Copy link
Contributor

@HazAT – just verifying, but this shouldn't conflict with getsentry/react-native-sentry, correct? (It has its own Raven.js plugin.)

@graingert – thanks and sorry for the slowness.

@graingert graingert deleted the patch-3 branch May 15, 2017 17:35
@graingert
Copy link
Contributor Author

graingert commented May 15, 2017

@benvinegar hey no worries I'm just glad for the existence of the project!

@graingert
Copy link
Contributor Author

@benvinegar @HazAT yeah it looks like that should be updated too

@graingert
Copy link
Contributor Author

It won't break it but now it will behave slightly different.

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.

2 participants