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

Auto-Trace Version 2 has arrived! #8

Merged
merged 9 commits into from
Sep 16, 2016
Merged

Conversation

keithhalterman
Copy link
Contributor

@keithhalterman keithhalterman commented Sep 15, 2016

New Auto-Trace API - Packed full of tests and documentation

@@ -10,6 +10,10 @@ export function addGlobalMiddleware(middlewareFn) {
globalMiddlewares.push(middlewareFn)
}

export function removeAllGlobalMiddlewares(){
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mainly so that tests can reset the middleware; figured others may want to reset the middleware as well.

const err = rawError || new Error();
const middlewareErr = executeMiddleware(err, extraMiddlewares);
return wrapObjectWithError(middlewareErr);
export function syncStacktrace(callback = ()=>{}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I like this change -- we can still call the middleware properly without doing this by doing something like this:

const stacktraceErr = new Error();
const syncMiddlewareErrFunctions = executeAsyncMiddleware(stacktraceErr);
executeSyncMiddleware(syncMiddlewareErrFunctions, stacktraceErr)
return stacktraceErr;

So in the case of synchronous apis, the middleware is called with the same Error for both asyncErr and syncErr, but in the case of asynchronous apis, the middleware is given two separate errors. This is at least what I remember deciding on when talking with you and Bret.

What are your thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't remembered that decision, sounds like a good plan though, this simplifies syncStacktrace.

@keithhalterman keithhalterman changed the title inital v2 api changes v2 api changes Sep 15, 2016
@keithhalterman keithhalterman changed the title v2 api changes Auto-Trace Version 2 has arrived! Sep 16, 2016
Copy link
Contributor

@joeldenning joeldenning left a comment

Choose a reason for hiding this comment

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

I think it's ready! Nice work on this!

* @param {Object} extraContext String or Object to stringify and append to the error message
* @return {Error} Error with extraContext
*/
export function appendExtraContext(error, extraContext){
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this!

@joeldenning
Copy link
Contributor

🚶

@keithhalterman keithhalterman merged commit 44b8560 into master Sep 16, 2016
@keithhalterman keithhalterman deleted the feature/api-version-2 branch September 16, 2016 20:34
This was referenced Sep 16, 2016
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