-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(nextjs): Make NextApiHandler
type version-agnostic
#5737
fix(nextjs): Make NextApiHandler
type version-agnostic
#5737
Conversation
8f4969f
to
33eb974
Compare
size-limit report 📦
|
Btw, do you know are there issues related to this we can close now? |
Not unless one of us has filed an issue about not being able to build a test app with Next 12.1.6+ while having a local copy of SDK linked in, which... I'll go look, but it'd surprise me. |
Oh I thought since the type we allow in Also quickly looked for issues but didn't find anything. 👍 |
33eb974
to
1058caf
Compare
1058caf
to
5fddc14
Compare
So, the issue is API page code like this: import { NextApiHandler } from 'next';
import { withSentry } from '@sentry/nextjs';
const handler: NextApiHandler = ...
export default withSentry(handler);
Scenario 1: User app
Scenario 2: SDK dev test app before this change
Scenario 3: SDK dev test app after this change
Does that make sense? |
Ah right. That makes a lot of sense. Thanks for the detailed explanation and also thanks for updating that comment! |
In
withSentry
in the nextjs SDK, we import and use theNextApiHandler
type provided by Next. In vercel/next.js#35166, which was released in 12.1.6, that type switched to usinguknown
rather thanvoid
for the function's return type.For users this isn't a problem. If they import both the type from Nextjs and
withSentry
from our SDK (which in turn imports the type), both imports will be pointed at their local version of Next, and the types will match. For us, though, it's a different story. Our dev version of Next is the lowest one the SDK supports, 10.0.8. If we link our SDK into a test app, theNextApiHandler
we're importing will be coming from thenode_modules
in our repo, whereas the test app will be pulling the type from its ownnode_modules
. And that works just fine... until you try to upgrade your test app to Next 12.1.6 or higher, at which point your app starts refusing to build because of the mismatch.(See #5737 (comment) for a more detailed explanation of the above.)
Though at first it might seem that the obvious solution is just to update our dev version of Next to 12.1.6+, if we do that we'll just be back in the same place with the same problem trying to use lower versions of Next in our test apps. Thus we need the type to work for both older and newer versions, which is what this PR does.