Skip to content

Commit

Permalink
Fix Sentry/Express tracing issues when using app.use() (#440)
Browse files Browse the repository at this point in the history
## Summary
<!-- Succinctly describe your change, providing context, what you've
changed, and why. -->

Express performs some runtime checks to assess functions being used to
handle middleware. When used with Express Sentry middleware (which wraps
other endpoints, like Inngest's), our internal use of variadic arguments
can produce a function signature that looks like it can't adequately
handle a request.

This should be resolved by using the signature of the function passed
when creating the serve handler, ensuring we keep the function signature
looking healthy.

Thanks for the repro of this issue, @spastorelli! Looks fixed using this
PR (`inngest@pr-440`).

- Original repro:
https://replit.com/@spsgitbook/InngestWithSentryRepro#index.js
- Fixed repro:
https://replit.com/@jpwilliams1/InngestWithSentryRepro#index.js
`npm install inngest@pr-440`

## Checklist
<!-- Tick these items off as you progress. -->
<!-- If an item isn't applicable, ideally please strikeout the item by
wrapping it in "~~"" and suffix it with "N/A My reason for skipping
this." -->
<!-- e.g. "- [ ] ~~Added tests~~ N/A Only touches docs" -->

- [ ] ~~Added a [docs PR](https://github.com/inngest/website) that
references this PR~~ N/A Bug fix
- [x] Added unit/integration tests
- [x] Added changesets if applicable

## Related

- Fixes #436
- getsentry/sentry-javascript#3284
  • Loading branch information
jpwilliams committed Dec 19, 2023
1 parent cb953ee commit 0fc642d
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 4 deletions.
5 changes: 5 additions & 0 deletions .changeset/fresh-waves-live.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"inngest": patch
---

Fix an issue where Sentry's wrapping of `inngest/express` caused Sentry to throw a runtime error during instantiation
4 changes: 2 additions & 2 deletions packages/inngest/etc/inngest.api.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 25 additions & 1 deletion packages/inngest/src/components/InngestCommHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ export class InngestCommHandler<
* ```
*/
public createHandler(): (...args: Input) => Promise<Awaited<Output>> {
return async (...args: Input) => {
const handler = async (...args: Input) => {
const timer = new ServerTiming();

/**
Expand Down Expand Up @@ -608,6 +608,30 @@ export class InngestCommHandler<
});
});
};

/**
* Some platforms check (at runtime) the length of the function being used
* to handle an endpoint. If this is a variadic function, it will fail
* that check.
*
* Therefore, we expect the arguments accepted to be the same length as
* the `handler` function passed internally.
*
* We also set a name to avoid a common useless name in tracing such as
* `"anonymous"` or `"bound function"`.
*
* https://github.com/getsentry/sentry-javascript/issues/3284
*/
Object.defineProperties(handler, {
name: {
value: "InngestHandler",
},
length: {
value: this.handler.length,
},
});

return handler;
}

/**
Expand Down
4 changes: 3 additions & 1 deletion packages/inngest/src/deno/fresh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ export const serve = (options: ServeHandlerOptions) => {

const fn = handler.createHandler();

return (req: Request) => fn(req, Deno.env.toObject());
return function handleRequest(req: Request) {
return fn(req, Deno.env.toObject());
};
};

declare const Deno: { env: { toObject: () => { [index: string]: string } } };
21 changes: 21 additions & 0 deletions packages/inngest/src/test/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,27 @@ export const testFramework = (
test("serve should be a function", () => {
expect(handler.serve).toEqual(expect.any(Function));
});

test("serve should return a function with a name", () => {
const actual = handler.serve({ client: inngest, functions: [] });

expect(actual.name).toEqual(expect.any(String));
expect(actual.name).toBeTruthy();
});

/**
* Some platforms check (at runtime) the length of the function being used
* to handle an endpoint. If this is a variadic function, it will fail
* that check.
*
* Therefore, we expect the arguments accepted to be the same length as
* the `handler` function passed internally.
*/
test("serve should return a function with a non-variadic length", () => {
const actual = handler.serve({ client: inngest, functions: [] });

expect(actual.length).toBeGreaterThan(0);
});
});

if (opts?.handlerTests) {
Expand Down

0 comments on commit 0fc642d

Please sign in to comment.