-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add support for native promises #28
Conversation
var originalThen = Promise.prototype.then | ||
Promise.prototype.then = wrappedThen | ||
|
||
function wrappedThen() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a SyntaxError in node 0.11.16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh interesting, I guess v8 strict mode has changed a bit between iojs 1.0.4 and 0.11.16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think since block scoping is on by default, having function declarations inside if
is ok
I added a few more tests, and everything seems to be working as expected. One thing I have not figured out is how to detect native promises vs some shim. It is native code so toString will hint at that, but its not really any different than if you call .bind on a non native function. Anyone have thoughts on this? |
Okay, I think detection for native promises is now pretty good, and all of the Promise methods are now tested, found a couple that are not in the spec or documented anywhere. |
What's the status of this? From what I've been able to figure out, getting this PR merged is key to using CLS with Koa. |
Should be good to go, just waiting on someone to review it
|
} | ||
} | ||
|
||
wrap(Promise.prototype, 'then', wrapThen) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed, catch appears to call then
@@ -0,0 +1,619 @@ | |||
if (!global.Promise) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any versions of node/io that might be broken in their promise support because of half baked implementations? Such as 0.11 with --harmony
, if so, do we care?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none without flags. Not sure if we care about flags, but the instrumentation is pretty straight forward, unless they were completely different, I don't see this being an issue.
👍 |
LGTM. I'd add a comment explaining the logic behind the weird interaction between resolve/reject and then calls though. It might be somewhat non-obvious what all this does to someone looking at the code in the future. |
This tries to implement what was discussed in #25