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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/nextjs/src/config/loaders/proxyLoader.ts
Expand Up @@ -7,6 +7,7 @@ import { LoaderThis } from './types';

type LoaderOptions = {
pagesDir: string;
pageExtensionRegex: string;
};

/**
Expand All @@ -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.


// Get the parameterized route name from this page's filepath
const parameterizedRoute = path
Expand All @@ -25,7 +26,7 @@ export default async function proxyLoader(this: LoaderThis<LoaderOptions>, userC
// Add a slash at the beginning
.replace(/(.*)/, '/$1')
// Pull off the file extension
.replace(/\.(jsx?|tsx?)/, '')
.replace(new RegExp(`\\.(${pageExtensionRegex})`), '')
// Any page file named `index` corresponds to root of the directory its in, URL-wise, so turn `/xyz/index` into
// just `/xyz`
.replace(/\/index$/, '')
Expand Down
2 changes: 2 additions & 0 deletions packages/nextjs/src/config/types.ts
Expand Up @@ -32,6 +32,8 @@ export type NextConfigObject = {
basePath?: string;
// Config which will be available at runtime
publicRuntimeConfig?: { [key: string]: unknown };
// File extensions that count as pages in the `pages/` directory
pageExtensions?: string[];
};

export type UserSentryOptions = {
Expand Down
8 changes: 6 additions & 2 deletions packages/nextjs/src/config/webpack.ts
Expand Up @@ -82,13 +82,17 @@ export function constructWebpackConfigFunction(
if (userSentryOptions.autoInstrumentServerFunctions) {
const pagesDir = newConfig.resolve?.alias?.['private-next-pages'] as string;

// Default page extensions per https://github.com/vercel/next.js/blob/f1dbc9260d48c7995f6c52f8fbcc65f08e627992/packages/next/server/config-shared.ts#L161
const pageExtensions = userNextConfig.pageExtensions || ['tsx', 'ts', 'jsx', 'js'];
const pageExtensionRegex = pageExtensions.map(escapeStringForRegex).join('|');

newConfig.module.rules.push({
// Nextjs allows the `pages` folder to optionally live inside a `src` folder
test: new RegExp(`${escapeStringForRegex(projectDir)}(/src)?/pages/.*\\.(jsx?|tsx?)`),
test: new RegExp(`${escapeStringForRegex(projectDir)}(/src)?/pages/.*\\.(${pageExtensionRegex})`),
use: [
{
loader: path.resolve(__dirname, 'loaders/proxyLoader.js'),
options: { pagesDir },
options: { pagesDir, pageExtensionRegex },
},
],
});
Expand Down
1 change: 1 addition & 0 deletions packages/nextjs/test/integration/next.config.js
Expand Up @@ -4,6 +4,7 @@ const moduleExports = {
eslint: {
ignoreDuringBuilds: true,
},
pageExtensions: ['jsx', 'js', 'tsx', 'ts', 'page.tsx'],
sentry: {
autoInstrumentServerFunctions: true,
// Suppress the warning message from `handleSourcemapHidingOptionWarning` in `src/config/webpack.ts`
Expand Down
1 change: 1 addition & 0 deletions packages/nextjs/test/integration/next10.config.template
Expand Up @@ -5,6 +5,7 @@ const moduleExports = {
future: {
webpack5: %RUN_WEBPACK_5%,
},
pageExtensions: ['jsx', 'js', 'tsx', 'ts', 'page.tsx'],
sentry: {
autoInstrumentServerFunctions: true,
// Suppress the warning message from `handleSourcemapHidingOptionWarning` in `src/config/webpack.ts`
Expand Down
1 change: 1 addition & 0 deletions packages/nextjs/test/integration/next11.config.template
Expand Up @@ -6,6 +6,7 @@ const moduleExports = {
eslint: {
ignoreDuringBuilds: true,
},
pageExtensions: ['jsx', 'js', 'tsx', 'ts', 'page.tsx'],
sentry: {
autoInstrumentServerFunctions: true,
// Suppress the warning message from `handleSourcemapHidingOptionWarning` in `src/config/webpack.ts`
Expand Down
@@ -0,0 +1,12 @@
const BasicPage = (): JSX.Element => (
<h1>
This page simply exists to test the compatibility of Next.js' `pageExtensions` option with our auto wrapping
process. This file should be turned into a page by Next.js and our webpack loader should process it.
</h1>
);

export async function getServerSideProps() {
return { props: { data: '[some getServerSideProps data]' } };
}

export default BasicPage;
@@ -0,0 +1,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.
Comment on lines +1 to +3
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.

@@ -0,0 +1,36 @@
const assert = require('assert');

const { sleep } = require('../utils/common');
const { getAsync, interceptTracingRequest } = require('../utils/server');

module.exports = async ({ url: urlBase, argv }) => {
const url = `${urlBase}/customPageExtension`;

const capturedRequest = interceptTracingRequest(
{
contexts: {
trace: {
op: 'http.server',
status: 'ok',
},
},
transaction: '/customPageExtension',
transaction_info: {
source: 'route',
changes: [],
propagations: 0,
},
type: 'transaction',
request: {
url,
},
},
argv,
'tracingServerGetServerSidePropsCustomPageExtension',
);

await getAsync(url);
await sleep(250);

assert.ok(capturedRequest.isDone(), 'Did not intercept expected request');
};