-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(nextjs): Take pageExtensions
option into account for auto instrumentation
#5881
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
Conversation
@@ -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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 thepageExtensions
option what files are turned into pages by Next.js.