fix(knex): make stack traces work in 0.18+ #1497
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Qard
added a commit
to Qard/apm-agent-nodejs
that referenced
this pull request
Nov 1, 2019
Qard
added a commit
that referenced
this pull request
Nov 1, 2019
watson
reviewed
Nov 1, 2019
| @@ -32,7 +32,7 @@ function logger () { | |||
| } | |||
|
|
|||
| function isFunction (funktion) { | |||
| return funktion && {}.toString.call(funktion) === '[object Function]' | |||
| return typeof funktion === 'function' | |||
There was a problem hiding this comment.
@Qard Wow, nice find!
This was also updated in the upstream shimmer module last year: othiym23/shimmer@ec15ba2
I wonder why typeof wasn't used originally.
Why was this making knex fail btw? I would love to know what knex did to fail this check 🤔 Did they use async or generator functions?
There was a problem hiding this comment.
They started using async functions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I figured out how to get stack traces working properly in knex. It was a result of knex switching from Bluebird to native promises in 0.18. In the process, they also switched to using async functions, which our version of shimmer apparently was unable to patch correctly because it used a
toString()check to identify functions rather than a simpletypeof, and async functions have a different name than regular functions.Checklist