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

feat(nextjs): Change not to dependent on @sentry/tracing #3903

Closed
wants to merge 1 commit into from
Closed

feat(nextjs): Change not to dependent on @sentry/tracing #3903

wants to merge 1 commit into from

Conversation

sudosubin
Copy link

In some project, @sentry/tracing with @sentry/hub is too heavy for browser bundle size. (~9kB) I wanted to remove @sentry/tracing from index.client.ts since I do not use tracing/performance, but it was really hard.

This PR make @sentry/tracing as an optional dependency if the project wants. With this change, user can easily opt-out @sentry/tracing like below.

// next.config.js
const { withSentryConfig } = require("@sentry/nextjs");

module.exports = withSentryConfig({
  webpack: (config, options) => {
    // Removes tracing from client bundle
    if (!options.isServer) {
      config.resolve.alias['@sentry/tracing'] = false;
    }
  }
})

I had to mock @sentry/next.js/dist/index.client.js to remove @sentry/tracing from bundle without this PR changes. It will be great if there exists any other better, or simpler solutions :)

@sudosubin sudosubin requested a review from kamilogorek as a code owner August 18, 2021 11:18
@AbhiPrasad
Copy link
Member

Thanks for your contribution! You change makes sense, I wonder if we can even include webpack alias ourselves and just expose a boolean option to disable tracing.

@lobsterkatie What do you think?

@AbhiPrasad AbhiPrasad added the Package: nextjs Issues related to the Sentry Nextjs SDK label Aug 18, 2021
@kamilogorek kamilogorek requested review from lobsterkatie and removed request for kamilogorek August 24, 2021 09:07
@lobsterkatie
Copy link
Member

lobsterkatie commented Aug 31, 2021

Actually, better (IMHO) would be to tie inclusion of the tracing package to tracing being enabled in the init options. The trick will be reading that value (ideally without running Sentry.init()) and figuring out how to deal with the fact that the user's config file might be in typescript. (We can't do a dynamic import because it can't be transpiled into ES6.)

UPDATE: Have been playing around with it and it's janky, but I think it wouldn't be that hard to just regex it. Or we could get fancy and transpile and then require it in a subprocess or something...

Anyway, I like the thinking here, but in the long run I'd like us to handle it within the SDK, rather than making the user mess with webpack config. I've made a ticket for us to think about doing so. In the meantime, I'll keep playing around with this. It's not actually working for me yet, but who knows with webpack.

@lobsterkatie
Copy link
Member

lobsterkatie commented Sep 14, 2021

Okay, got it to work, and I've applied it (in a small way) in #3978. I still would like to investigate dropping the whole tracing package for those not using it. One way or another I think we can figure out how to do it.

Thank you for the PR, though, and the inspiration for the above change (and possibly future changes)! 🙂

@lobsterkatie
Copy link
Member

UPDATE: I was confusing this PR with your other one when I closed this. Sorry! I'll take another look at this change tomorrow.

@lobsterkatie lobsterkatie reopened this Sep 15, 2021
Comment on lines +12 to 13
const { BrowserTracing } = TracingIntegrations || {};
export const Integrations = { ...BrowserIntegrations, BrowserTracing };
Copy link
Member

@lobsterkatie lobsterkatie Sep 15, 2021

Choose a reason for hiding this comment

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

I'd like us to handle this better in the future, but for now, please just add this TODO and edit to the export const Integrations line.

Suggested change
const { BrowserTracing } = TracingIntegrations || {};
export const Integrations = { ...BrowserIntegrations, BrowserTracing };
// TODO: The `|| {}` here is a workaround to enable people not using tracing to reduce bundle size by using
// `resolve.alias["@sentry/tracing"] = false` in their webpack config to tell webpack 5+ to replace the tracing package
// with an empty object. If they do that , `TracingIntegrations` will be undefined and this destructuring will fail
// without the empty object being provided as an alternative from which to unpack. In the long run, we should handle
// this internally.
const { BrowserTracing } = TracingIntegrations || {};
export const Integrations = { ...BrowserIntegrations, ...(BrowserTracing && {BrowserTracing}) };

@github-actions
Copy link
Contributor

This pull request 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 🥀

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
None yet
Development

Successfully merging this pull request may close these issues.

4 participants