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

fix(nextjs): Support automatic instrumentation for app directory with custom page extensions #12858

Merged

Conversation

ziyadkhalil
Copy link
Contributor

@ziyadkhalil ziyadkhalil commented Jul 10, 2024

This PR updates the regex and logic for detecting middleware, route handler, and server component files to support custom page extensions, such as middleware.custom.ts and route.custom.ts.

Changes Made

  • Modified the isRouteHandler function to use a regex that matches custom page extensions for route handler files.
  • Modified the isMiddlewareResource function to use a regex that matches custom page extensions for middleware files.
  • Modified the isServerComponent function to use a regex that matches custom page extensions for server component files.
  • modified hasGlobalErrorFile flag to account for custom page extensions

Testing

  • Verified the updated regex matches route handlers and middleware files with standard and custom extensions.
  • Added new cases for server loaders tests to cover route handler files
  • Updated and passed all test cases.
  • Added a duplicate e2e nextjs project with page extensions enabled and it passes all tests e2e(nextjs): add nextjs app dir project with page extensions #12864

Checklist


Please review and provide feedback. Thank you!

@ziyadkhalil ziyadkhalil changed the title fix(nextjs): Support automatic instrumentation for route handlers and… fix(nextjs): Support automatic instrumentation for route handlers and middleware with custom page extensions Jul 10, 2024
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ziyadkhalil thanks for the PR! Could you update an e2e test to validate this? https://github.com/getsentry/sentry-javascript/tree/develop/dev-packages/e2e-tests

ziyadkhalil added a commit to ziyadkhalil/sentry-javascript that referenced this pull request Jul 10, 2024
@ziyadkhalil ziyadkhalil force-pushed the ziyad/fix-route-handlers-instrumentation branch from 317caa7 to 295e85b Compare July 10, 2024 18:44
@ziyadkhalil
Copy link
Contributor Author

@ziyadkhalil thanks for the PR! Could you update an e2e test to validate this? https://github.com/getsentry/sentry-javascript/tree/develop/dev-packages/e2e-tests

@AbhiPrasad Sure! I'm not sure which test to update tho as to test this change we have to set the pageExtension option in next.config.js.
Should I add a new nextjs project under test applications with the page extension option set in next.config.js and have the same tests as nextjs-app-dir?

Thank you for the quick feedback!

@AbhiPrasad
Copy link
Member

Sure! I'm not sure which test to update tho as to test this change we have to set the pageExtension option in next.config.js

Good question. I'll let the other sdk maintainers chime in here for this as I have no strong opinions

@ziyadkhalil ziyadkhalil force-pushed the ziyad/fix-route-handlers-instrumentation branch from 295e85b to 432ecb0 Compare July 10, 2024 21:53
ziyadkhalil added a commit to ziyadkhalil/sentry-javascript that referenced this pull request Jul 10, 2024
@ziyadkhalil ziyadkhalil force-pushed the ziyad/fix-route-handlers-instrumentation branch from 432ecb0 to bb20466 Compare July 10, 2024 22:48
@ziyadkhalil ziyadkhalil changed the title fix(nextjs): Support automatic instrumentation for route handlers and middleware with custom page extensions fix(nextjs): Support automatic instrumentation for app directory with custom page extensions Jul 10, 2024
@ziyadkhalil
Copy link
Contributor Author

@AbhiPrasad Great! I created another PR #12864 with the suggested solution and it was a great idea that made me notice that page extensions feature is not working for server components as well.
I updated this PR to enable instrumentation for server components as well.
It passes all the e2e tests in the other PR #12864

@ziyadkhalil ziyadkhalil requested a review from AbhiPrasad July 10, 2024 23:15
@lforst lforst self-requested a review July 11, 2024 07:22
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Comment on lines -174 to -175
resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/app/nested/page.ts', // ts is not a valid file ending for pages in the app dir
expectedWrappingTargetKind: undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked and this is actually not true. It's just very uncommon because usually you have jsx in your pages.

@lforst
Copy link
Member

lforst commented Jul 11, 2024

I am gonna merge this so that this is off your plate @ziyadkhalil. I will add tests as a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants