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

ref(nextjs): Remove next.js plugin #3462

Merged
merged 5 commits into from
Apr 28, 2021
Merged

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Apr 27, 2021

NB: This PR was accidentally merged without the complete set of changes described below being included. The remaining changes are in #3476.


This PR removes the nextjs plugin, in light of the infrastructure for it having been removed by Vercel, and replaces it with a combination of webpack loaders and server monkeypatches. Specifically:

  • In order to ensure that the SDK is initialized (in other words, that sentry.server.config.js and sentry.client.config.js are run) when the server starts and a page loads in the browser, respectively, the entry property in the webpack config is modified to include those files in three places: 1) in pages/_document, which covers non-API pages, 2) in pages/api/*, which covers API request handlers, and 3) main, which is included in every page loaded in the browser.

  • To catch errors in API routes, a new withSentry helper has been introduced, with which API routes can be wrapped. Docs for that are here. We will try to make this automatic in the future.

  • To catch errors in functions like getServerSideProps (functions which run as part of non-API request handling), the server is monkeypatched to wrap the error logger which is called whenever such an error occurs.

There are still a number of TODOs, as you will see in the code, but none of them are blockers, and can therefore be handled in future PRs.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 27, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 20.63 KB (+0.01% 🔺)
@sentry/browser - Webpack 21.5 KB (0%)
@sentry/react - Webpack 21.53 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 27.92 KB (0%)

Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

😢

@kamilogorek
Copy link
Contributor

🕊️

…ack (#3463)

Co-authored-by: Daniel Griesser <daniel.griesser.86@gmail.com>
@HazAT HazAT changed the title ref(nextjs): Remove plugin ref(nextjs): Remove next.js plugin Apr 27, 2021
packages/nextjs/src/utils/handlers.ts Show resolved Hide resolved
scope.addEventProcessor(event => parseRequest(event, req));
captureException(e);
});
await flush(2000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is flushing on every error required? #3301 (comment)

Choose a reason for hiding this comment

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

@iker-barriocanal @lobsterkatie i've found that flushing here breaks Next.js API Routes in dev mode when using cors middleware.

It results in an ERR_STREAM_WRITE_AFTER_END error.

Removing the call to flush resolves it, but i'm not sure what the side affects of doing this might be.

Copy link
Member Author

Choose a reason for hiding this comment

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

We flush to ensure that when API routes are deployed as serverless functions, the event gets sent before the function ends. @ashconnell - can you share a sample API route which is crashing for you?

Copy link

@ashconnell ashconnell May 12, 2021

Choose a reason for hiding this comment

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

@lobsterkatie here's a reproduction.

I'm using the latest Next.js and Sentry versions.
To reproduce the error you need to fetch the api route from another origin so that it triggers the cors preflight etc.

Note that the request works if you remove the withSentry wrapper.

import { withSentry } from '@sentry/nextjs'
import Cors from 'cors'

// This is straight from Next.js docs on how to use cors
// See: https://nextjs.org/docs/api-routes/api-middlewares#connectexpress-middleware-support
function runMiddleware(req, res, fn) {
  return new Promise((resolve, reject) => {
    fn(req, res, result => {
      if (result instanceof Error) {
        return reject(result)
      }

      return resolve(result)
    })
  })
}

const cors = async (req, res) => {
  return await runMiddleware(req, res, Cors({}))
}

async function handler(req, res) {
  await cors(req, res)
  res.status(200).send({ message: 'Hello!' })
}

export default withSentry(handler)

Copy link

@ashconnell ashconnell May 12, 2021

Choose a reason for hiding this comment

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

@lobsterkatie from what I can tell, flush is somehow calling res.end() after the regular request has already ended. Since flush is part of the core, maybe regular express apps protect against calling .end() multtiple times, but Next.js just uses a thin wrapper around node's http.ServerResponse which does not

Copy link

@ashconnell ashconnell May 12, 2021

Choose a reason for hiding this comment

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

I may have found something. So when @sentry/nextjs calls flush, @sentry/node attempts to get the client which does not exist, and rejects the promise with false

// node/sdk.ts
export async function flush(timeout?: number): Promise<boolean> {
  const client = getCurrentHub().getClient<NodeClient>();
  if (client) {
    return client.flush(timeout);
  }
  return Promise.reject(false);
}

Since the @sentry/nextjs flush awaits flush in the finally clause and it fails, the whole withSentry call is rejected, which Next.js catches and calls res.end().

This is where Next.js handles this: https://github.com/vercel/next.js/blob/canary/packages/next/next-server/server/api-utils.ts#L84

When making the exact same request from the same origin, the client value is defined so this error never happens. I have no idea why this is.

packages/nextjs/package.json Show resolved Hide resolved
// type WebpackExport = (config: WebpackConfig, options: WebpackOptions) => Promise<WebpackConfig>;

// The two arguments passed to the exported `webpack` function, as well as the thing it returns
type WebpackConfig = { devtool: string; plugins: PlainObject[]; entry: EntryProperty };

Choose a reason for hiding this comment

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

Maybe use type Configuration which is exported from webpack? By the way, it probably should be added to peer dependencies (and also to dev dependencies if the type Configuration is used)... It's also good from the perspective of letting users know what version of webpack this package expects.

Choose a reason for hiding this comment

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

Next is in the process of moving from webpack 4 to webpack 5 right now. Worth taking that into account with the dep versioning.

@lobsterkatie lobsterkatie deleted the kmclb-remove-nextjs-plugin branch April 28, 2021 07:17
@HazAT HazAT restored the kmclb-remove-nextjs-plugin branch April 28, 2021 09:07
@HazAT HazAT reopened this Apr 28, 2021
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.

7 participants