Skip to content

fix: next.js few fixes #3479

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

Merged
merged 8 commits into from
Apr 29, 2021
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@

- "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott

## 6.3.4

- [nextjs] fix: API routes logging (#3479)

## 6.3.3

- [nextjs] fix: User server types (#3471)
Expand Down
67 changes: 35 additions & 32 deletions packages/nextjs/src/utils/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ type WebpackExport = (config: WebpackConfig, options: WebpackOptions) => Webpack

// The two arguments passed to the exported `webpack` function, as well as the thing it returns
type WebpackConfig = { devtool: string; plugins: PlainObject[]; entry: EntryProperty };
// TODO use real webpack types
type WebpackOptions = { dev: boolean; isServer: boolean };
type WebpackOptions = { dev: boolean; isServer: boolean; buildId: string };

// For our purposes, the value for `entry` is either an object, or a function which returns such an object
type EntryProperty = (() => Promise<EntryPropertyObject>) | EntryPropertyObject;
Expand All @@ -27,33 +26,10 @@ type EntryProperty = (() => Promise<EntryPropertyObject>) | EntryPropertyObject;
type EntryPropertyObject = PlainObject<string | Array<string> | EntryPointObject>;
type EntryPointObject = { import: string | Array<string> };

const injectSentry = async (origEntryProperty: EntryProperty, isServer: boolean): Promise<EntryProperty> => {
// Out of the box, nextjs uses the `() => Promise<EntryPropertyObject>)` flavor of EntryProperty, where the returned
// object has string arrays for values. But because we don't know whether someone else has come along before us and
// changed that, we need to check a few things along the way.

// The `entry` entry in a webpack config can be a string, array of strings, object, or function. By default, nextjs
// sets it to an async function which returns the promise of an object of string arrays. Because we don't know whether
// someone else has come along before us and changed that, we need to check a few things along the way. The one thing
// we know is that it won't have gotten *simpler* in form, so we only need to worry about the object and function
// options. See https://webpack.js.org/configuration/entry-context/#entry.

let newEntryProperty = origEntryProperty;

if (typeof origEntryProperty === 'function') {
newEntryProperty = await origEntryProperty();
}

newEntryProperty = newEntryProperty as EntryPropertyObject;

// according to vercel, we only need to inject Sentry in one spot for server and one spot for client, and because
// those are used as bases, it will apply everywhere
const injectionPoint = isServer ? 'pages/_document' : 'main';
const injectee = isServer ? './sentry.server.config.js' : './sentry.client.config.js';

/** Add a file (`injectee`) to a given element (`injectionPoint`) of the `entry` property */
const _injectFile = (entryProperty: EntryPropertyObject, injectionPoint: string, injectee: string): void => {
// can be a string, array of strings, or object whose `import` property is one of those two
let injectedInto = newEntryProperty[injectionPoint];

let injectedInto = entryProperty[injectionPoint];
// whatever the format, add in the sentry file
injectedInto =
typeof injectedInto === 'string'
Expand All @@ -73,15 +49,42 @@ const injectSentry = async (origEntryProperty: EntryProperty, isServer: boolean)
: // array case for inner property
[injectee, ...injectedInto.import],
};
entryProperty[injectionPoint] = injectedInto;
};

newEntryProperty[injectionPoint] = injectedInto;

const injectSentry = async (origEntryProperty: EntryProperty, isServer: boolean): Promise<EntryProperty> => {
// Out of the box, nextjs uses the `() => Promise<EntryPropertyObject>)` flavor of EntryProperty, where the returned
// object has string arrays for values. But because we don't know whether someone else has come along before us and
// changed that, we need to check a few things along the way.
// The `entry` entry in a webpack config can be a string, array of strings, object, or function. By default, nextjs
// sets it to an async function which returns the promise of an object of string arrays. Because we don't know whether
// someone else has come along before us and changed that, we need to check a few things along the way. The one thing
// we know is that it won't have gotten *simpler* in form, so we only need to worry about the object and function
// options. See https://webpack.js.org/configuration/entry-context/#entry.
let newEntryProperty = origEntryProperty;
if (typeof origEntryProperty === 'function') {
newEntryProperty = await origEntryProperty();
}
newEntryProperty = newEntryProperty as EntryPropertyObject;
// On the server, we need to inject the SDK into both into the base page (`_document`) and into individual API routes
// (which have no common base).
if (isServer) {
Object.keys(newEntryProperty).forEach(key => {
if (key === 'pages/_document' || key.includes('pages/api')) {
// for some reason, because we're now in a function, we have to cast again
_injectFile(newEntryProperty as EntryPropertyObject, key, './sentry.server.config.js');
}
});
}
// On the client, it's sufficient to inject it into the `main` JS code, which is included in every browser page.
else {
_injectFile(newEntryProperty, 'main', './sentry.client.config.js');
}
// TODO: hack made necessary because the async-ness of this function turns our object back into a promise, meaning the
// internal `next` code which should do this doesn't
if ('main.js' in newEntryProperty) {
delete newEntryProperty['main.js'];
}

return newEntryProperty;
};

Expand All @@ -97,7 +100,6 @@ export function withSentryConfig(
providedWebpackPluginOptions: Partial<SentryCliPluginOptions> = {},
): NextConfigExports {
const defaultWebpackPluginOptions = {
release: getSentryRelease(),
url: process.env.SENTRY_URL,
org: process.env.SENTRY_ORG,
project: process.env.SENTRY_PROJECT,
Expand Down Expand Up @@ -144,6 +146,7 @@ export function withSentryConfig(
// TODO it's not clear how to do this better, but there *must* be a better way
new ((SentryWebpackPlugin as unknown) as typeof defaultWebpackPlugin)({
dryRun: options.dev,
release: getSentryRelease(options.buildId),
...defaultWebpackPluginOptions,
...providedWebpackPluginOptions,
}),
Expand Down
4 changes: 3 additions & 1 deletion packages/nextjs/src/utils/instrumentServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ function makeWrappedErrorLogger(origErrorLogger: ErrorLogger): WrappedErrorLogge
return (err: Error): void => {
// TODO add context data here
Sentry.captureException(err);
return origErrorLogger(err);
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
return origErrorLogger.bind(this, err);
};
}
5 changes: 3 additions & 2 deletions packages/node/src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ export async function close(timeout?: number): Promise<boolean> {
/**
* Returns a release dynamically from environment variables.
*/
export function getSentryRelease(): string | undefined {
export function getSentryRelease(fallback?: string): string | undefined {
// Always read first as Sentry takes this as precedence
if (process.env.SENTRY_RELEASE) {
return process.env.SENTRY_RELEASE;
Expand All @@ -176,6 +176,7 @@ export function getSentryRelease(): string | undefined {
// Zeit (now known as Vercel)
process.env.ZEIT_GITHUB_COMMIT_SHA ||
process.env.ZEIT_GITLAB_COMMIT_SHA ||
process.env.ZEIT_BITBUCKET_COMMIT_SHA
process.env.ZEIT_BITBUCKET_COMMIT_SHA ||
fallback
);
}
15 changes: 12 additions & 3 deletions packages/serverless/src/awslambda.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,15 @@ export function tryPatchHandler(taskRoot: string, handlerPath: string): void {
(mod as HandlerModule)[functionName!] = wrapHandler(obj as Handler);
}

/**
* Tries to invoke context.getRemainingTimeInMillis if not available returns 0
* Some environments use AWS lambda but don't support this function
* @param context
*/
function tryGetRemainingTimeInMillis(context: Context): number {
return typeof context.getRemainingTimeInMillis === 'function' ? context.getRemainingTimeInMillis() : 0;
}

/**
* Adds additional information from the environment and AWS Context to the Sentry Scope.
*
Expand All @@ -155,7 +164,7 @@ function enhanceScopeWithEnvironmentData(scope: Scope, context: Context, startTi
function_version: context.functionVersion,
invoked_function_arn: context.invokedFunctionArn,
execution_duration_in_millis: performance.now() - startTime,
remaining_time_in_millis: context.getRemainingTimeInMillis(),
remaining_time_in_millis: tryGetRemainingTimeInMillis(context),
'sys.argv': process.argv,
});

Expand Down Expand Up @@ -221,7 +230,7 @@ export function wrapHandler<TEvent, TResult>(
context.callbackWaitsForEmptyEventLoop = options.callbackWaitsForEmptyEventLoop;

// In seconds. You cannot go any more granular than this in AWS Lambda.
const configuredTimeout = Math.ceil(context.getRemainingTimeInMillis() / 1000);
const configuredTimeout = Math.ceil(tryGetRemainingTimeInMillis(context) / 1000);
const configuredTimeoutMinutes = Math.floor(configuredTimeout / 60);
const configuredTimeoutSeconds = configuredTimeout % 60;

Expand All @@ -233,7 +242,7 @@ export function wrapHandler<TEvent, TResult>(
// When `callbackWaitsForEmptyEventLoop` is set to false, which it should when using `captureTimeoutWarning`,
// we don't have a guarantee that this message will be delivered. Because of that, we don't flush it.
if (options.captureTimeoutWarning) {
const timeoutWarningDelay = context.getRemainingTimeInMillis() - options.timeoutWarningLimit;
const timeoutWarningDelay = tryGetRemainingTimeInMillis(context) - options.timeoutWarningLimit;

timeoutWarningTimer = setTimeout(() => {
withScope(scope => {
Expand Down