Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Make capture* callbacks fire after sending, more API cleanup/singletonization #217

Merged
merged 5 commits into from
Oct 25, 2016

Conversation

LewisJEllis
Copy link
Contributor

Short version: addresses #213, moves the API to follow raven-js almost exactly, still maintains BC in almost all ways with old API

Details:

  • captureException etc now fire their callbacks after sending to the server, as in Fix the API around capture* and handle callback correctly #213
  • Moved everything to a Raven class instead of Client, following raven-js pattern/API
    • We now export a default Raven instance, leading to almost-identical usage to raven-js
    • Still have a Client wrapper for bc/multi-instance capabilities
  • Does some other polishing/cleanups on the API changes
  • Adds glue to maintain backwards compat with old API as much as possible
    • None of the changes to tests actually affect any existing public API usage patterns at all

The only public API usage affected by this should be the one described by @mattrobenolt in #213, and the realistic caveat to run into from it is that existing code looking at eventIds from capture* callbacks will now get null instead of a string.

/cc @benvinegar

@benvinegar
Copy link
Contributor

Looks okay, but it's hard to see through the indentation.

My only ask is: a test that demonstrates that the callback behavior has changed to triggering after send (vs before send).

@LewisJEllis
Copy link
Contributor Author

@benvinegar pushed a commit for that.

message: 'test',
}, function () {
firedCallback = true;
sentResponse.should.equal(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this test guarantee that done() is called after the process callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, had mixed up with a change made in one of the other PRs and this wasn't checking it correctly. Fixed.

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

Successfully merging this pull request may close these issues.

2 participants