sentry-javascript icon indicating copy to clipboard operation
sentry-javascript copied to clipboard

Tree-shaking browserTracingIntegration in NextJS

Open antongunkin opened this issue 1 year ago • 8 comments

Is there an existing issue for this?

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

How do you use Sentry?

Self-hosted/on-premise

Which SDK are you using?

@sentry/nextjs

SDK Version

8

Framework Version

Next 14.2.10

Link to Sentry event

No response

Reproduction Example/SDK Setup

Hey!

Can't get rid of extra browserTracingIntegration integration. As I can see it happened here: https://github.com/getsentry/sentry-javascript/releases/tag/8.26.0

It worked (-50kb size-limit) prior this version. Could you please check?

Thank you!

Image

Steps to Reproduce

sentry.client.config.ts

Sentry.init({
  enableTracing: false,
});

next.config.js

config.plugins.push(
  new webpack.DefinePlugin({
    __SENTRY_TRACING__: false,
  }),
);

const sentryConfig = {
  bundleSizeOptimizations: {
    excludeTracing: true,
  },
};

module.exports = module.exports = withSentryConfig(nextConfig, sentryConfig);

Expected Result

No browserTracingIntegration.js in clients js-bundle.

Actual Result

Extra integration, increased bundle-size.

antongunkin avatar Oct 17 '24 17:10 antongunkin

Hmm, you can omit enableTracing: false, this should not be necessary.

I just tried in our own size-limit setup, and it seems to be correct to me like this:

{
    name: '@sentry/nextjs (without tracing)',
    path: 'packages/nextjs/build/esm/client/index.js',
    import: '{ init }',
    ignore: ['next/router', 'next/constants'],
    gzip: true,
    limit: '39 KB',
    modifyWebpackConfig: function (config) {
      const webpack = require('webpack');
      const TerserPlugin = require('terser-webpack-plugin');

      config.plugins.push(
        new webpack.DefinePlugin({
          __SENTRY_TRACING__: false,
        }),
      );

      config.optimization.minimize = true;
      config.optimization.minimizer = [new TerserPlugin()];

      return config;
    },
  },

is smaller than without the webpack config, in the range that I would expect.

You wrote:

It worked (-50kb size-limit) prior this version.

Do you mean that with version 8.25.0 it worked for you, and then not anymore?

mydea avatar Oct 17 '24 17:10 mydea

Hey @mydea !

Do you mean that with version 8.25.0 it worked for you, and then not anymore?

Correct! Only 8.25.0 (and below) works well. And yes, enableTracing: false does nothing.

🤷‍♂️

antongunkin avatar Oct 17 '24 18:10 antongunkin

Hmm, and the only change you did there was to bump the version from 8.25.0 to 8.26.0 - no other (e.g. config) changes?

I don't immediately see anything in https://github.com/getsentry/sentry-javascript/compare/8.25.0...8.26.0 that jumps out, and our size limit seems to still tree shake properly 🤔 cc @lforst or @chargome could you try this in some "real" next app to verify that tree shaking works on the latest version of the SDK?

mydea avatar Oct 17 '24 18:10 mydea

@mydea Thanks for checking!

What I see in 8.26.0:

  • https://github.com/getsentry/sentry-javascript/pull/13324
  • https://github.com/getsentry/sentry-javascript/issues/13010

Plus extra non-documented options here: https://github.com/getsentry/sentry-javascript/pull/13323/files#diff-338f827c7c3625622b0b38a45cd130517a9f872f6de2d354a7c80785462dcc3dR122 (which also didn't help)

antongunkin avatar Oct 17 '24 18:10 antongunkin

Yeah, but if tree shaking is properly set up, it should still remove this due to __SENTRY_TRACING__ being false 🤔

mydea avatar Oct 17 '24 18:10 mydea

@antongunkin can you 100% verify that the only change you're making before and after it works is bumping the SDK? No other code changes?

I just tried our tree-shaking capabilities in my test app and browserTracingIntegration properly tree-shakes as soon as I add

  webpack: (config, { webpack }) => {
    config.plugins.push(
      new webpack.DefinePlugin({
        __SENTRY_TRACING__: false,
      }),
    );
    return config;
  },

Maybe try to use the webpack instance that nextjs gives you here instead of requiring it! I tested with the exact same versions you mentioned. Maybe also try to upgrade everything to the latest version.

lforst avatar Oct 18 '24 11:10 lforst

Hey @mydea and @lforst , thanks again for checking! I just tried different project with same next and sdk versions and it works without any changes... So I can confirm that problem on my side only. Will let you know soon, maybe it's just some node_modules conflicts e.g.

antongunkin avatar Oct 18 '24 12:10 antongunkin

Ok, fixed, but it was weird.

I had a lazy (next/dynamic) component with additionally lazy session replays ES6 import:

import('@sentry/nextjs').then((lazyLoadedSentry) => {
  Sentry.addIntegration(
    lazyLoadedSentry.replayIntegration({ ... }),
  );
});

Fixed by replacing just to:

Sentry.addIntegration(
  Sentry.replayIntegration({ ... }),
);

It's strange that Webpack couldn't resolve it since 8.26.0. Also worth to check this documentation: https://docs.sentry.io/platforms/javascript/guides/nextjs/session-replay/#lazy-loading-replay

And thank you!

antongunkin avatar Oct 18 '24 15:10 antongunkin

Thanks for the pointer to update the docs. Doing import('@sentry/nextjs') indeed inhibits tree shaking because dynamic imports will always pull in the entire dependency.

lforst avatar Oct 21 '24 07:10 lforst

Looking at it more, this might be a webpack/nextjs config issue because I would assume webpack to kinda duplicate the bundles. Honestly, it might also just be a bundle analyzer thing.

lforst avatar Oct 21 '24 07:10 lforst