feat(nextjs): Add auto-wrapping for server components #6953
Conversation
| @@ -11,7 +11,7 @@ export default [ | |||
| 'src/client/index.ts', | |||
| 'src/server/index.ts', | |||
| 'src/edge/index.ts', | |||
| 'src/config/webpack.ts', | |||
| 'src/config/index.ts', | |||
There was a problem hiding this comment.
This was an oversight from before
lforst
force-pushed
the
lforst-add-app-directory-component-wrappers
branch
from
5e9bb35
to
fdf9300
Compare
size-limit report 📦
|
…omponent-wrappers
…ectory-component-wrappers
lforst
requested review from
mydea and
AbhiPrasad
and removed request for
mydea and
AbhiPrasad
| */ | ||
| export function wrapAppDirComponentWithSentry<F extends (...args: any[]) => any>(appDirComponent: F): F { | ||
| // Even though users may define server components as async functions, for the client bundles | ||
| // Next.js will turn them into synchronous functionsf and it will transform any`await`s into instances of the`use` |
There was a problem hiding this comment.
| // Next.js will turn them into synchronous functionsf and it will transform any`await`s into instances of the`use` | |
| // Next.js will turn them into synchronous functions and it will transform any `await`s into instances of the`use` |
Ditto for the one below.
I don't like repeating this logic between server/edge, but I guess it's fine for now.
There was a problem hiding this comment.
I wonder what kind of spans we could get from here 🤔
There was a problem hiding this comment.
We could probably get some nice ones but since async context is super funky here atm with the app dir we can't have only one transaction for all the server components and I would feel bad for creating individual transactions for each server component.
(We need to put in some more work here - I will create an issue to track everything that we still can improve)
| loader: path.resolve(__dirname, 'loaders', 'wrappingLoader.js'), | ||
| options: { | ||
| ...staticWrappingLoaderOptions, | ||
| wrappingTargetKind: 'middleware', |
There was a problem hiding this comment.
m: Ideally I would like to see some unit tests that show we are applying the correct wrappingTargetKind - this seems fragile and easy to regress on if we change the path test.
There was a problem hiding this comment.
Added some in 9401d66
Thanks for holding me accountable to tests btw.
Ref: #6726
This PR adds a very basic wrapper for Next.js app directory server components that reports errors that occur. Additionally, this PR adds auto wrapping for that wrapper and a test to verify the auto wrapping.
I did some slight refactoring of the wrapping loader because it was becoming unwieldy with multiple different wrapping types.
Tests for app dir are broken atm so not adding any right now.