Conditional require calls are hoisted incorrectly

[beckerei]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/nextjs

SDK Version

7.33.0

Framework Version

Next 13.1.5

Link to Sentry event

No response

SDK Setup

  Sentry.init({
    dsn: 'foobar',
    environment,
    enabled: !!environment && ['staging', 'production'].includes(environment),
    tracesSampleRate: 0.2,
    tracesSampler: (ctx) => !ctx.transactionContext.name.includes('healthz'),
    beforeSend(event) {
      if (!event.request?.cookies) return event;
      const { cookies } = event.request;

      Object.keys(cookies).forEach((key) => {
        if (key.toLowerCase().includes('session')) {
          cookies[key] = '[Redacted]';
        }
      });

      return event;
    },
  });

Steps to Reproduce

1. conditionally use a require() call
2. start prod or dev server of next in a case where the condition should be false

Expected Result

The file should not be required

Actual Result

The file will be required regardless of the surrounding if-statement

---

Our understanding is that this behavior was introduced within the following PR
#6685

Enabeling transformMixedEsModules
See also rollup/plugins#1177 which indicates that the rollup plugin can't fix this.

This is the recommended way to run MSW in your local env, see example https://github.com/vercel/next.js/blob/canary/examples/with-msw/pages/_app.tsx

[lforst]

Hi @beckerei, thanks for the detailed writeup and research. Unfortunately, I am not sure if we can fix this fast and reliably, if at all. If you have suggestions I am all ears.

Would doing a dynamic import work for you? From what I can see here the underlying call is using dynamic imports anyways so it should work if I am not mistaken.

[joshuajaco]

Yes, using a dynamic import in that case does solve the issue. However this is not a sustainable solution. Next supports conditional require calls, therefore so should sentry, especially since it already supported it prior to version 7.31.0.

I think the only reliable solution is to keep the original import/export syntax and let the next build handle transformation. I'm afraid this will be hard to accomplish with rollup, if possible at all.

[lforst]

@joshuajaco I have a feeling the whole ecosystem is moving away from CJS/require as a whole so I think we should let this simmer for a bit and see if more people run into this problem before rushing to a fix.

Can I ask why you deem using dynamic imports a non-sustainable solution?

[joshuajaco]

In this case I agree, a dynamic import is actually the better solution. Beside the fact that you might need to import something synchronously, my biggest concern is actually that nextjs supports require out of the box, but this breaks when installing sentry. This is totally unexpected and confusing behaviour that is difficult to debug. Not to mention that this might not even be something you notice until it is too late. In our case we suddenly started showing mock data on production.

I think a warning message as mentioned in rollup/plugins#1177 (comment) would already help prevent this issue from remaining unnoticed.

[beckerei]

@lforst thank you for taking care :)

[lforst]

@beckerei No worries. Let's hope this actually fixes your issue. I'll try not to forget to ping everybody once this is released.

[lforst]

Hi, we just released an update that should fix this problem. https://github.com/getsentry/sentry-javascript/releases/tag/7.35.0

Let me know if upgrading the SDK fixes this issue for you!

[joshuajaco]

Works as expected now 👍