getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.97k stars 1.57k forks source link

`@sentry/nextjs` throws `withSentry is not a function` #3750

Closed Vadorequest closed 2 years ago

Vadorequest commented 3 years ago

Package + Version

Version:

0.0.0

Description

Using Sentry.withSentry fails using "@sentry/nextjs": "6.7.2", I tried other versions, but the results were the same.

Source code is open source and available at https://github.com/UnlyEd/next-right-now/pull/370.

Reproduction:

image

import { logEvent } from '@/modules/core/amplitude/amplitudeServerClient';
import {
  AMPLITUDE_API_ENDPOINTS,
  AMPLITUDE_EVENTS,
} from '@/modules/core/amplitude/events';
import { filterExternalAbsoluteUrl } from '@/modules/core/js/url';
import { createLogger } from '@/modules/core/logging/logger';
import { configureReq } from '@/modules/core/sentry/server';
import { flushSafe } from '@/modules/core/sentry/universal';
import * as Sentry from '@sentry/nextjs';
import appendQueryParameter from 'append-query';
import {
  NextApiRequest,
  NextApiResponse,
} from 'next';

const fileLabel = 'api/preview';
const logger = createLogger({
  fileLabel,
});

/**
 * Key to use in order to force disable "auto preview mode".
 *
 * If a page is loaded with noAutoPreviewMode=true in query parameter, then it won't try to enable the Preview mode, even if it's disabled.
 *
 * @example ?noAutoPreviewMode=true
 */
export const NO_AUTO_PREVIEW_MODE_KEY = 'noAutoPreviewMode';

type EndpointRequestQuery = {
  /**
   * Whether to start/stop the Preview Mode.
   *
   * @example ?stop=true Will stop the preview mode.
   * @example ?stop=false Will start the preview mode.
   * @default ?stop=false
   */
  stop: string;

  /**
   * Url to redirect to once the preview mode has been started/stopped.
   *
   * @example ?redirectTo=/en
   * @example ?redirectTo=/fr/solutions
   * @default ?redirectTo=/
   */
  redirectTo: string;
}

type EndpointRequest = NextApiRequest & {
  query: EndpointRequestQuery;
};

/**
 * Preview Mode API.
 *
 * Enables and disables preview mode.
 *
 * The official example uses a security token to enable the preview mode, we don't.
 * This is a choice, as we don't need/want to protect our preview mode.
 * Protecting the preview mode makes most sense when this mode can be used in production, so that you can preview content served by Next.js from a CMS/tool of your choice.
 * Thus, it's strongly related to how you're planning on using it, and we decided to keep it simpler, by not using any kind of security.
 *
 * @param req
 * @param res
 * @method GET
 *
 * @see https://nextjs.org/docs/advanced-features/preview-mode#step-1-create-and-access-a-preview-api-route
 * @see https://nextjs.org/docs/advanced-features/preview-mode#clear-the-preview-mode-cookies
 */
export const preview = async (req: EndpointRequest, res: NextApiResponse): Promise<void> => {
  try {
    configureReq(req, { fileLabel });

    await logEvent(AMPLITUDE_EVENTS.API_INVOKED, null, {
      apiEndpoint: AMPLITUDE_API_ENDPOINTS.PREVIEW,
    });

    const {
      stop = 'false',
      redirectTo = '/',
    }: EndpointRequestQuery = req.query;
    // Add NO_AUTO_PREVIEW_MODE_KEY parameter to query, to avoid running into infinite loops if the Preview mode couldn't start
    // Useful when the cookie created by Next.js cannot be written (Incognito mode)
    const safeRedirectUrl = appendQueryParameter(filterExternalAbsoluteUrl(redirectTo as string), `${NO_AUTO_PREVIEW_MODE_KEY}=true`);

    // XXX We don't want to enable preview mode for the production stage, it's only allowed for non-production stages
    //  It's allowed during development for testing purpose
    //  It's allowed during staging because this stage is being used as a "preview environment"
    if (process.env.NEXT_PUBLIC_APP_STAGE !== 'production') {
      if (stop === 'true') {
        res.clearPreviewData();

        logger.info('Preview mode stopped');
      } else {
        res.setPreviewData({});

        logger.info('Preview mode enabled');
      }
    } else {
      logger.error('Preview mode is not allowed in production');
      Sentry.captureMessage('Preview mode is not allowed in production', Sentry.Severity.Warning);
    }

    await flushSafe();

    res.writeHead(307, { Location: safeRedirectUrl });
    res.end();
  } catch (e) {
    Sentry.captureException(e);
    logger.error(e.message);

    await flushSafe();

    res.json({
      error: true,
      message: process.env.NEXT_PUBLIC_APP_STAGE === 'production' ? undefined : e.message,
    });
  }
};

export default Sentry.withSentry(preview);
lobsterkatie commented 3 years ago

Hi, @Vadorequest. I've pinged you in other issues about various things, so forgive me if some of this is repetition - I just want there to be an answer in case anyone else lands here in the future.

I tried running your repro code and got the following:

Cloning into 'nrn-next-sentry'...
The authenticity of host 'github.com (192.30.255.112)' can't be established.
RSA key fingerprint is SHA256:nThbg6kXUpJWGl7E1IGOCspRomTxdCARLviKw6E5SY8.
Are you sure you want to continue connecting (yes/no/[fingerprint])? yes
Warning: Permanently added 'github.com,192.30.255.112' (RSA) to the list of known hosts.
git@github.com: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

so I wasn't able to play around with it. That said, I see that this is from a few versions ago, and some of the comments in the code of that PR reference issues in this repo which are now closed. Can you please confirm if you're still having this particular issue when using the latest version of the SDK? You should be able to eliminate most of the sentry-related boilerplate in your example above - the captureException, flushSafe, and res.end calls - because all of those are covered in withSentry.

Please let me know!

ka2n commented 3 years ago

I got the same error when I implemented the API route in a slightly different pattern than normal. (Importing API route's exports from /pages)

repo: https://github.com/ka2n/sentry-nextjs-example

github-actions[bot] commented 3 years ago

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

karna41317 commented 3 years ago

I still have this issue as @ka2n and @Vadorequest mentioned. I can reproduce with latest packages


├─ @sentry/cli@1.69.1
├─ @sentry/integrations@6.13.3
├─ @sentry/nextjs@6.13.3
├─ @sentry/node@6.13.3
├─ @sentry/react@6.13.3
└─ @sentry/webpack-plugin@1.17.1
Vadorequest commented 3 years ago

I didn't take the time to follow-up on https://github.com/getsentry/sentry-javascript/issues/3750#issuecomment-888818093 comment, but as far as I know it hasn't been solved for me either. (I didn't try since last time, though)

Vadorequest commented 2 years ago

@lobsterkatie I think you tried to clone at a time when GitHub was down.

It works fine for me, and the repository has always been public so it shouldn't be an issue to clone any branch.

 ✔  git clone git@github.com:UnlyEd/next-right-now.git nrn-next-sentry && cd nrn-next-sentry && git checkout next-sentry && cp .env.local.example .env.local && yarn && yarn start
Cloning into 'nrn-next-sentry'...
remote: Enumerating objects: 18770, done.
remote: Counting objects: 100% (4719/4719), done.
remote: Compressing objects: 100% (1517/1517), done.
remote: Total 18770 (delta 3220), reused 4576 (delta 3090), pack-reused 14051
Receiving objects: 100% (18770/18770), 22.98 MiB | 2.05 MiB/s, done.
Resolving deltas: 100% (13153/13153), done.
Branch 'next-sentry' set up to track remote branch 'next-sentry' from 'origin'.
Switched to a new branch 'next-sentry'
yarn install v1.22.10
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
...
lobsterkatie commented 2 years ago

Likely related to https://github.com/getsentry/sentry-javascript/issues/3673.

vladanpaunovic commented 2 years ago

A lot of work has happened into the @sentry/nextjs Is this issue still present with newer versions of our SDKs?

lobsterkatie commented 2 years ago

@vladanpaunovic The related issue I linked seems still to be active, so this one may be as well.

@ka2n, I suspect you may still be running into this, because your repro app above (thanks for that!) is set up somewhat like the repro in that issue, in that you're calling a function from pages/api/xxxxx.js directly. Having now looked at both repros again, I'm thinking that's the issue (the other repro had additional problems, but this is something the two have in common).

Now that I'm thinking about it, I does make (some) sense. wtithSentry is server-side only, and really shouldn't be in the client bundle (which is where you're calling fetchHello from, at least when SSR isn't happening). What's confusing to me is that neither it nor the handler are getting treeshaken out. Nothing in the client bundle is calling it, so I'd expect it to get dropped.

(I saw your docstring, but I don't think it's correct. When I build your app (using yarn build, not yarn dev), both are still included, as you can see at the bottom of this screenshot:

image

The handler's been anonymized, but it's still there.)

Regardless, I tried moving fetchHello into a separate file, and that prevented withSentry from showing up in the index page bundle, so I think that's your solution here.

I'm considering adding a note to the docs about this, but before I do, I want to get a sense of how common a use case this is. Is setting it up that way something you came to on your own, or were you following a guide of some sort?

ka2n commented 2 years ago

@lobsterkatie I'm sorry for the short response. I guess I didn't do enough research on the tree shaking issue, thank you! This technique is not that major, and it got a bit of a boost in my local community(ref), but it's still not a major method locally, so I don't think it's worth mentioning.

lobsterkatie commented 2 years ago

Got it, thanks.

I'm going to close this then, as it seems like we've come to a working solution.

Cheers!

zodman commented 2 years ago

I can reproduce these error. and still showed with the last version of sentry.

image

For reproducing use the simple implementation of sentry: https://github.com/zodman/nextjs-with-sentry

The problem is in my case when you export a variable in the API directory https://github.com/zodman/nextjs-with-sentry/blob/main/pages/api/test1.js#L3 and reuse on the pages: https://github.com/zodman/nextjs-with-sentry/blob/main/pages/page1.js#L2

long story short .. you mixing the backend with the frontend ...