Skip to content

Commit

Permalink
🐛 Fix instrumention of null function with 3rd party wrapper (#1570) (#…
Browse files Browse the repository at this point in the history
…1697)

* Wrap function to protect against nulls/undefined

A simple solution for #1570. It feels like we could also just skip wrapping anything that isn't a function on L11, and immediately returning a no-op stop, but I was less comfortable making that change.

* add unit tests for instrument method when null

* fix type to allow null return

* change to undefined and a clearer order

* move test and rename
  • Loading branch information
Ryankey authored Aug 25, 2022
1 parent 985b210 commit 40e4d60
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 3 deletions.
18 changes: 16 additions & 2 deletions packages/core/src/tools/instrumentMethod.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,26 @@ describe('instrumentMethod', () => {

expect(instrumentationSpy).not.toHaveBeenCalled()
})

it('should not throw errors if original method was undefined', () => {
const object: { method?: () => number } = {}
const instrumentationStub = () => 2
const { stop } = instrumentMethod(object, 'method', () => instrumentationStub)

thirdPartyInstrumentation(object)

stop()

expect(object.method).not.toThrow()
})
})
})

function thirdPartyInstrumentation(object: { method: () => number }) {
function thirdPartyInstrumentation(object: { method?: () => number }) {
const originalMethod = object.method
object.method = () => originalMethod() + 2
if (typeof originalMethod === 'function') {
object.method = () => originalMethod() + 2
}
}
})

Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/tools/instrumentMethod.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ export function instrumentMethod<OBJECT extends { [key: string]: any }, METHOD e

let instrumentation = instrumentationFactory(original)

const instrumentationWrapper = function (this: OBJECT): ReturnType<OBJECT[METHOD]> {
const instrumentationWrapper = function (this: OBJECT): ReturnType<OBJECT[METHOD]> | undefined {
if (typeof instrumentation !== 'function') {
return undefined
}
// eslint-disable-next-line @typescript-eslint/no-unsafe-return
return instrumentation.apply(this, arguments as unknown as Parameters<OBJECT[METHOD]>)
}
Expand Down

0 comments on commit 40e4d60

Please sign in to comment.