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.

Sign up for GitHub

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

Jump to bottom

fix(nextjs): Inject init calls via loader instead of via entrypoints #8368

Merged
merged 8 commits into from
Jun 21, 2023
Merged

fix(nextjs): Inject init calls via loader instead of via entrypoints #8368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3963559
YEET
lforst Jun 20, 2023
10bc5b3
Separate loader
lforst Jun 20, 2023
e2da315
.
lforst Jun 20, 2023
168272e
..
lforst Jun 20, 2023
f281e30
Fix tests
lforst Jun 20, 2023
49b85e3
...
lforst Jun 20, 2023
beb04de
lint
lforst Jun 20, 2023
1d928a5
Bump timeout a bit
lforst Jun 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ jobs:
name: Nextjs (Node ${{ matrix.node }}) Tests
needs: [job_get_metadata, job_build]
if: needs.job_get_metadata.outputs.changed_nextjs == 'true' || github.event_name != 'pull_request'
timeout-minutes: 15
timeout-minutes: 25
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary or did you change it for testing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately the tests timed out :/

runs-on: ubuntu-20.04
strategy:
fail-fast: false
Expand Down
18 changes: 9 additions & 9 deletions packages/nextjs/src/config/loaders/wrappingLoader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,21 +201,21 @@ export default function wrappingLoader(
} else {
templateCode = templateCode.replace(/__COMPONENT_TYPE__/g, 'Unknown');
}

// We check whether `this.resourcePath` is absolute because there is no contract by webpack that says it is absolute,
// however we can only create relative paths to the sentry config from absolute paths.Examples where this could possibly be non - absolute are virtual modules.
if (sentryConfigFilePath && path.isAbsolute(this.resourcePath)) {
const sentryConfigImportPath = path
.relative(path.dirname(this.resourcePath), sentryConfigFilePath) // Absolute paths do not work with Windows: https://github.com/getsentry/sentry-javascript/issues/8133
.replace(/\\/g, '/');
templateCode = `import "${sentryConfigImportPath}";\n`.concat(templateCode);
}
} else if (wrappingTargetKind === 'middleware') {
templateCode = middlewareWrapperTemplateCode;
} else {
throw new Error(`Invariant: Could not get template code of unknown kind "${wrappingTargetKind}"`);
}

// We check whether `this.resourcePath` is absolute because there is no contract by webpack that says it is absolute,
// however we can only create relative paths to the sentry config from absolute paths.Examples where this could possibly be non - absolute are virtual modules.
if (sentryConfigFilePath && path.isAbsolute(this.resourcePath)) {
const sentryConfigImportPath = path
.relative(path.dirname(this.resourcePath), sentryConfigFilePath) // Absolute paths do not work with Windows: https://github.com/getsentry/sentry-javascript/issues/8133
.replace(/\\/g, '/');
templateCode = `import "${sentryConfigImportPath}";\n`.concat(templateCode);
}

// Replace the import path of the wrapping target in the template with a path that the `wrapUserCode` function will understand.
templateCode = templateCode.replace(/__SENTRY_WRAPPING_TARGET_FILE__/g, WRAPPING_TARGET_MODULE_NAME);

Expand Down
42 changes: 8 additions & 34 deletions packages/nextjs/src/config/webpack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-disable complexity */
/* eslint-disable max-lines */
import { getSentryRelease } from '@sentry/node';
import { arrayify, dropUndefinedKeys, escapeStringForRegex, logger, stringMatchesSomePattern } from '@sentry/utils';
import { arrayify, dropUndefinedKeys, escapeStringForRegex, logger } from '@sentry/utils';
import { default as SentryWebpackPlugin } from '@sentry/webpack-plugin';
import * as chalk from 'chalk';
import * as fs from 'fs';
Expand Down Expand Up @@ -441,7 +441,7 @@ async function addSentryToEntryProperty(

// inject into all entry points which might contain user's code
for (const entryPointName in newEntryProperty) {
if (shouldAddSentryToEntryPoint(entryPointName, runtime, userSentryOptions.excludeServerRoutes ?? [])) {
if (shouldAddSentryToEntryPoint(entryPointName, runtime)) {
addFilesToExistingEntryPoint(newEntryProperty, entryPointName, filesToInject);
} else {
if (
Expand Down Expand Up @@ -589,39 +589,13 @@ function checkWebpackPluginOverrides(
* @param excludeServerRoutes A list of excluded serverside entrypoints provided by the user
* @returns `true` if sentry code should be injected, and `false` otherwise
*/
function shouldAddSentryToEntryPoint(
entryPointName: string,
runtime: 'node' | 'browser' | 'edge',
excludeServerRoutes: Array<string | RegExp>,
): boolean {
// On the server side, by default we inject the `Sentry.init()` code into every page (with a few exceptions).
if (runtime === 'node') {
// User-specified pages to skip. (Note: For ease of use, `excludeServerRoutes` is specified in terms of routes,
// which don't have the `pages` prefix.)
const entryPointRoute = entryPointName.replace(/^pages/, '');
if (stringMatchesSomePattern(entryPointRoute, excludeServerRoutes, true)) {
return false;
}

// This expression will implicitly include `pages/_app` which is called for all serverside routes and pages
// regardless whether or not the user has a`_app` file.
return entryPointName.startsWith('pages/');
} else if (runtime === 'browser') {
return (
// entrypoint for `/pages` pages - this is included on all clientside pages
// It's important that we inject the SDK into this file and not into 'main' because in 'main'
// some important Next.js code (like the setup code for getCongig()) is located and some users
// may need this code inside their Sentry configs
entryPointName === 'pages/_app' ||
function shouldAddSentryToEntryPoint(entryPointName: string, runtime: 'node' | 'browser' | 'edge'): boolean {
return (
runtime === 'browser' &&
(entryPointName === 'pages/_app' ||
// entrypoint for `/app` pages
entryPointName === 'main-app'
);
} else {
// User-specified pages to skip. (Note: For ease of use, `excludeServerRoutes` is specified in terms of routes,
// which don't have the `pages` prefix.)
const entryPointRoute = entryPointName.replace(/^pages/, '');
return !stringMatchesSomePattern(entryPointRoute, excludeServerRoutes, true);
}
entryPointName === 'main-app')
);
}

/**
Expand Down
149 changes: 0 additions & 149 deletions packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,7 @@ import {
CLIENT_SDK_CONFIG_FILE,
clientBuildContext,
clientWebpackConfig,
EDGE_SDK_CONFIG_FILE,
edgeBuildContext,
exportedNextConfig,
SERVER_SDK_CONFIG_FILE,
serverBuildContext,
serverWebpackConfig,
userNextConfig,
Expand Down Expand Up @@ -88,143 +85,22 @@ describe('constructWebpackConfigFunction()', () => {
});

describe('webpack `entry` property config', () => {
const serverConfigFilePath = `./${SERVER_SDK_CONFIG_FILE}`;
const clientConfigFilePath = `./${CLIENT_SDK_CONFIG_FILE}`;
const edgeConfigFilePath = `./${EDGE_SDK_CONFIG_FILE}`;

it('handles various entrypoint shapes', async () => {
const finalWebpackConfig = await materializeFinalWebpackConfig({
exportedNextConfig,
incomingWebpackConfig: serverWebpackConfig,
incomingWebpackBuildContext: serverBuildContext,
});

expect(finalWebpackConfig.entry).toEqual(
expect.objectContaining({
// original entrypoint value is a string
// (was 'private-next-pages/_error.js')
'pages/_error': [serverConfigFilePath, 'private-next-pages/_error.js'],

// original entrypoint value is a string array
// (was ['./node_modules/smellOVision/index.js', 'private-next-pages/sniffTour.js'])
'pages/sniffTour': [
serverConfigFilePath,
'./node_modules/smellOVision/index.js',
'private-next-pages/sniffTour.js',
],

// original entrypoint value is an object containing a string `import` value
// (was { import: 'private-next-pages/api/simulator/dogStats/[name].js' })
'pages/api/simulator/dogStats/[name]': {
import: [serverConfigFilePath, 'private-next-pages/api/simulator/dogStats/[name].js'],
},

// original entrypoint value is an object containing a string array `import` value
// (was { import: ['./node_modules/dogPoints/converter.js', 'private-next-pages/simulator/leaderboard.js'] })
'pages/simulator/leaderboard': {
import: [
serverConfigFilePath,
'./node_modules/dogPoints/converter.js',
'private-next-pages/simulator/leaderboard.js',
],
},

// original entrypoint value is an object containg properties besides `import`
// (was { import: 'private-next-pages/api/tricks/[trickName].js', dependOn: 'treats', })
'pages/api/tricks/[trickName]': {
import: [serverConfigFilePath, 'private-next-pages/api/tricks/[trickName].js'],
dependOn: 'treats', // untouched
},
}),
);
});

it('injects user config file into `_app` in server bundle and in the client bundle', async () => {
const finalServerWebpackConfig = await materializeFinalWebpackConfig({
exportedNextConfig,
incomingWebpackConfig: serverWebpackConfig,
incomingWebpackBuildContext: serverBuildContext,
});
const finalClientWebpackConfig = await materializeFinalWebpackConfig({
exportedNextConfig,
incomingWebpackConfig: clientWebpackConfig,
incomingWebpackBuildContext: clientBuildContext,
});

expect(finalServerWebpackConfig.entry).toEqual(
expect.objectContaining({
'pages/_app': expect.arrayContaining([serverConfigFilePath]),
}),
);
expect(finalClientWebpackConfig.entry).toEqual(
expect.objectContaining({
'pages/_app': expect.arrayContaining([clientConfigFilePath]),
}),
);
});

it('injects user config file into `_error` in server bundle but not client bundle', async () => {
const finalServerWebpackConfig = await materializeFinalWebpackConfig({
exportedNextConfig,
incomingWebpackConfig: serverWebpackConfig,
incomingWebpackBuildContext: serverBuildContext,
});
const finalClientWebpackConfig = await materializeFinalWebpackConfig({
exportedNextConfig,
incomingWebpackConfig: clientWebpackConfig,
incomingWebpackBuildContext: clientBuildContext,
});

expect(finalServerWebpackConfig.entry).toEqual(
expect.objectContaining({
'pages/_error': expect.arrayContaining([serverConfigFilePath]),
}),
);
expect(finalClientWebpackConfig.entry).toEqual(
expect.objectContaining({
'pages/_error': expect.not.arrayContaining([clientConfigFilePath]),
}),
);
});

it('injects user config file into both API routes and non-API routes', async () => {
const finalWebpackConfig = await materializeFinalWebpackConfig({
exportedNextConfig,
incomingWebpackConfig: serverWebpackConfig,
incomingWebpackBuildContext: serverBuildContext,
});

expect(finalWebpackConfig.entry).toEqual(
expect.objectContaining({
'pages/api/simulator/dogStats/[name]': {
import: expect.arrayContaining([serverConfigFilePath]),
},

'pages/api/tricks/[trickName]': expect.objectContaining({
import: expect.arrayContaining([serverConfigFilePath]),
}),

'pages/simulator/leaderboard': {
import: expect.arrayContaining([serverConfigFilePath]),
},
}),
);
});

it('injects user config file into API middleware', async () => {
const finalWebpackConfig = await materializeFinalWebpackConfig({
exportedNextConfig,
incomingWebpackConfig: serverWebpackConfig,
incomingWebpackBuildContext: edgeBuildContext,
});

expect(finalWebpackConfig.entry).toEqual(
expect.objectContaining({
middleware: [edgeConfigFilePath, 'private-next-pages/middleware.js'],
}),
);
});

it('does not inject anything into non-_app pages during client build', async () => {
const finalWebpackConfig = await materializeFinalWebpackConfig({
exportedNextConfig,
Expand All @@ -244,30 +120,5 @@ describe('constructWebpackConfigFunction()', () => {
simulatorBundle: './src/simulator/index.ts',
});
});

it('does not inject into routes included in `excludeServerRoutes`', async () => {
const nextConfigWithExcludedRoutes = {
...exportedNextConfig,
sentry: {
excludeServerRoutes: [/simulator/],
},
};
const finalWebpackConfig = await materializeFinalWebpackConfig({
exportedNextConfig: nextConfigWithExcludedRoutes,
incomingWebpackConfig: serverWebpackConfig,
incomingWebpackBuildContext: serverBuildContext,
});

expect(finalWebpackConfig.entry).toEqual(
expect.objectContaining({
'pages/simulator/leaderboard': {
import: expect.not.arrayContaining([serverConfigFilePath]),
},
'pages/api/simulator/dogStats/[name]': {
import: expect.not.arrayContaining([serverConfigFilePath]),
},
}),
);
});
});
});