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): Take pageExtensions option into account for auto instrumentation #5881

Merged
merged 1 commit into from Oct 4, 2022

Conversation

lforst
Copy link
Member

@lforst lforst commented Oct 4, 2022

Fixes #5877

This PR makes our auto-wrapping of data fetchers aware of the pageExtensions option that users can provide in their Next.js config. Previously we simply wrapped all .tsx? and .jsx? files. This is problematic in case users narrow down with the pageExtensions option what files are turned into pages by Next.js.

@lforst lforst added this to the NextJS Improvements milestone Oct 4, 2022
@@ -16,7 +17,7 @@ type LoaderOptions = {
*/
export default async function proxyLoader(this: LoaderThis<LoaderOptions>, userCode: string): Promise<string> {
// We know one or the other will be defined, depending on the version of webpack being used
const { pagesDir } = 'getOptions' in this ? this.getOptions() : this.query;
const { pagesDir, pageExtensionRegex } = 'getOptions' in this ? this.getOptions() : this.query;
Copy link
Member

Choose a reason for hiding this comment

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

m: we don't have to set a default here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean it is always passed in very explicitly (see webpack.ts). Would it make sense to have another default here in your opinion?

Copy link
Member

Choose a reason for hiding this comment

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

Ahhh ok my bad didn't see how the types were linked up - with LoaderOptions it makes sense now.

@lforst lforst merged commit ff1229b into master Oct 4, 2022
@lforst lforst deleted the lforst-fix-nextjs-custom-page-extensions branch October 4, 2022 17:25
Comment on lines +1 to +3
This page simply exists to test the compatibility of Next.js' `pageExtensions` option with our auto wrapping
process. This file should not be turned into a page by Next.js and our webpack loader also shouldn't process it.
This page should not contain valid JavaScript.
Copy link
Member

Choose a reason for hiding this comment

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

Can we test the negative here in some way?

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured that this already is testing the negative - not really by having a dedicated test, but rather by not crashing when such a file is provided.

We could theoretically try and open this page and wait for a 404 but at that point, I feel like we're testing Next.js itself. Did you have something else in mind?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I guess it would cause the build to fail if we tried to wrap it, wouldn't it? Okay, I think we're good here, thanks.

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.

Can the 'next js-page extensions' be applied in @sentry/nextjs?
3 participants