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

Middleware should be provided to the serve function, not the client #438

Open
samtgarson opened this issue Dec 15, 2023 · 4 comments
Open
Assignees
Labels
⬆️ improvement Performance, reliability, or usability improvements 📦 inngest Affects the `inngest` package

Comments

@samtgarson
Copy link

Describe the bug
As far as I can tell, middleware is only actually executed during function runs, however we have to register them (globally, at least) in the client.

This is problematic if we need to use libraries which are not able to be imported in all the contexts that the inngest client might be used.

For example: @appsignal/nodejs is not able to be imported in the Next.js Edge runtime due to webpack issues, and we would like to be able to set tags and capture errors in functions using AppSignal. However, we also need to trigger functions using inngest.send in Edge functions and Next-Auth callbacks, which run in middleware (and therefore the Edge Runtime).

It would make sense to me (if my understanding is correct) that there's no reason the client needs to know about middleware at the point of triggering events, only at the point of running functions.

To Reproduce

  1. Register a middleware in the inngest client which imports a non-webpackable client, like import { setTags } from '@appsignal/nodejs'
  2. Call inngest.send from Next.js middleware
  3. See various webpack errors trying to import the AppSignal server package

Expected behavior
I should be able to trigger events without having to import server only packages.

Screenshots / Stack trace dump
If applicable, add screenshots or paste what is output in your terminal to help explain your problem.

System info

  • inngest-cli version: 0.23.0
  • inngest 3.6.2
@samtgarson
Copy link
Author

Just tried setting up a similar thing on a separate Next.js, this time using Sentry, and effectively copy and pasted your example Sentry middleware, and I'm getting the same issue with Edge functions importing Sentry.
image

Maybe I'm missing something on how to instantiate the Inngest client to trigger events without this?

@samtgarson
Copy link
Author

samtgarson commented Dec 15, 2023

(Update: importing @sentry/nextjs instead of @sentry/node seems to have solved that, but this will not always be a viable workaround for other libraries)

@jpwilliams jpwilliams transferred this issue from inngest/inngest Dec 19, 2023
@jpwilliams jpwilliams assigned jpwilliams and unassigned djfarrelly Dec 19, 2023
@jpwilliams jpwilliams added ⬆️ improvement Performance, reliability, or usability improvements 📦 inngest Affects the `inngest` package labels Dec 19, 2023
@jpwilliams
Copy link
Member

Hi @samtgarson 👋

Thanks for reporting. I empathise with the annoyance here.

Middleware can also be used to hook into the lifecycle for sending events with inngest.send(), as well as affecting typing for functions. With that, a single piece of middleware can control almost all of the SDK's I/O, hence being placed on the client, but it's a tripwire nonetheless.

In your case, I'd recommend creating and importing a separate client for use in the edge environment for sending events, which gives you the chance to ensure only relevant middleware is used in the more restrictive edge environment.

I'll add a change to our docs to call out this workaround.

We're not 100% happy with the interaction between new Inngest() and serve(). We have some changes in the works, but I'll leave this issue open to confirm the workaround is valid for you and discuss what some improvements might be here.

@samtgarson
Copy link
Author

Hey @jpwilliams thanks for the reply. I missed that middleware can hook into sending.

It didn't occur to me to make a second client, this seems to have worked (with a hefty comment to explain to other engineers when to use which client 😅)

Agreed, this isn't ideal—it might make sense to provide a second "serve only middleware" option to serve, or something similar. It seems to me that serve and send, while sharing some common configuration, have very different requirements?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⬆️ improvement Performance, reliability, or usability improvements 📦 inngest Affects the `inngest` package
Projects
None yet
Development

No branches or pull requests

3 participants