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

Tree-shaking browserTracingIntegration in NextJS #14011

Closed
3 tasks done
antongunkin opened this issue Oct 17, 2024 · 10 comments
Closed
3 tasks done

Tree-shaking browserTracingIntegration in NextJS #14011

antongunkin opened this issue Oct 17, 2024 · 10 comments
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK

Comments

@antongunkin
Copy link

antongunkin commented Oct 17, 2024

Is there an existing issue for this?

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.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 17, 2024
@github-actions github-actions bot added the Package: nextjs Issues related to the Sentry Nextjs SDK label Oct 17, 2024
@mydea
Copy link
Member

mydea commented Oct 17, 2024

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?

@antongunkin
Copy link
Author

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.

🤷‍♂️

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 17, 2024
@mydea
Copy link
Member

mydea commented Oct 17, 2024

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 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?

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 17, 2024
@mydea
Copy link
Member

mydea commented Oct 17, 2024

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

@lforst
Copy link
Member

lforst commented Oct 18, 2024

@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.

@antongunkin
Copy link
Author

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
Copy link
Author

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!

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 18, 2024
@lforst
Copy link
Member

lforst commented Oct 21, 2024

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
Copy link
Member

lforst commented Oct 21, 2024

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 lforst closed this as completed Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK
Projects
Archived in project
Development

No branches or pull requests

3 participants