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

Make it easier to expose Sentry API and tree shake at the same time #7713

Closed
rchl opened this issue Apr 3, 2023 · 14 comments
Closed

Make it easier to expose Sentry API and tree shake at the same time #7713

rchl opened this issue Apr 3, 2023 · 14 comments

Comments

@rchl
Copy link
Contributor

rchl commented Apr 3, 2023

Problem Statement

In Sentry Nuxt Module, there is a "Nuxt Context" object on which the module exposes the whole Sentry SDK API. It did that with a code like:

import * as Sentry from '@sentry/vue'
// ...
ctx.$sentry = Sentry

The problem with that was that it would import everything from @sentry/vue which includes a bunch of stuff that shouldn't need to be exposed to module users or stuff like @sentry/replay that users are not even using (user would have to manually import this one to use it).

That bloated the client bundle (build by webpack 4) which I haven't noticed until recently.

So my solution that I have already implemented (but I'm not entirely happy about) was to do more targeted importing like:

import { init, Integrations, vueRouterInstrumentation } from '~@sentry/vue'
import * as CoreSdk from '~@sentry/core'
import * as BrowserSdk from '~@sentry/browser-sdk'  // resolves to `@sentry/browser/esm/sdk.js`
// ...
const SentrySdk = { ...CoreSdk, ...BrowserSdk }
ctx.$sentry = Sentry

And that works to keep the bundle at bay.

The potential problem with that is that I'm just guessing here in regards to which APIs should actually be exported to the user. I've picked the ones from @sentry/core and exports from a specific file in @sentry/browser. This doesn't seem that future proof as Sentry could export new important APIs somewhere else and I wouldn't know about those.

Solution Brainstorm

Create some official list of APIs (methods/properties) that the user should have access to. Ideally have a specific export in the package (like @sentry/vue in this case) that lists all exports that should be exposed at runtime. Not including things like @sentry/replay, of course.

@lforst
Copy link
Member

lforst commented Apr 3, 2023

I will discuss this with the team but I think it is unlikely that we will maintain a lookup of "guaranteed" API in the future. My suggestion would be for you to do export * from '@sentry/vue'. We follow semver so as long as you pin the minor this should never break. This means however that it is very likely that you have to do major bumps whenever we decide to do one. Does this make sense?

@rchl
Copy link
Contributor Author

rchl commented Apr 3, 2023

Exporting from the Sentry Vue package is what I have been doing. The problem with that is that it pulls in a lot of exports that don't have to or even should be in the client bundle.

See my recent PR for the sentry Nuxt module to see how much difference it makes to not export all: nuxt-community/sentry-module#547

@Lms24
Copy link
Member

Lms24 commented Apr 4, 2023

The problem with that is that it pulls in a lot of exports that don't have to or even should be in the client bundle.

Can you elaborate which exports from @sentry/vue shouldn't end up in the client bundle in your opinion? I think it should be noted that just because certain e.g. integrations are exported from @sentry/vue, like Replay or BrowserTracingin recent versions, doesn't mean that they automatically end up in any bundle. For instance, both of the mentioned integrations should be tree-shaken out as long as users don't import and use them in their code (i.e. put it in Sentry.init). If this doesn't work, I suggest to take a look at your bundler setup. Perhaps it is not configured correctly for tree-shaking?

@rchl
Copy link
Contributor Author

rchl commented Apr 4, 2023

Please have a look at the initial comment again, especially at the first code block:

import * as Sentry from '@sentry/vue'
// ...
ctx.$sentry = Sentry

Webpack is not gonna be able to treeshake anything here because we are importing everything from the package and attaching it to the context which is exposed in user code.

I can give more details later on which specific exports i think should not be needed in user code but i don't have access to the code right now

@lforst
Copy link
Member

lforst commented Apr 4, 2023

We are aware that this will prevent Sentry from being tree-shakable. My question is why are you attaching it to the context instead of letting users import API directly?

The consensus in the team is that we will not maintain "essential API" for various reasons.

@rchl
Copy link
Contributor Author

rchl commented Apr 4, 2023

I suppose there are both philosophical and technical reasons for that.

Nuxt modules' approach is to provide a simple experience so that the user doesn't have to think where to import stuff from.

On the technical side, Nuxt is an universal framework so same Vue files are rendered both on the server and client side. So if the user would import from @sentry/node in a Vue file then that import would then be also included in the client bundle.

User could do some gymnastics and use async imports but that has drawbacks (separate network requests) and it's just not straight forward.

Nuxt module automatically exposes Vue exports on the client side and node exports on the server side on the same context object.

I understand that you don't want to add more complexity to your packages. I will just have to manually pick and choose the exports to expose then.

@rchl
Copy link
Contributor Author

rchl commented Apr 4, 2023

Btw. The Next Sentry package is probably the closest to the Nuxt module I'm maintaining so maybe someone from that team has some good tips for me? I wonder how does it handle the case of "universally" rendered react files.

@lforst
Copy link
Member

lforst commented Apr 4, 2023

@rchl I am currently more or less the owner of the Next.js SDK. For that SDK we hook into the bundling process and decide for the user which code to import depending on whether we're building server or client bundles.

This works by resolving "@sentry/nextjs" differently depending on what bundle is built. I elegantly (but at the cost of my team ridiculing me) named this process "SDK multiplexer". I guess since you're using webpack you could do the same thing. The "multiplexer" is just a very simple webpack loader.

@rchl
Copy link
Contributor Author

rchl commented Apr 4, 2023

But that sounds more or less like what I'm doing - picking and choosing specific exports to expose. Just in a slightly different way 🙂.

The advantage for you is that you work at Sentry so you are constantly aware of things changing and can adjust appropriately. ;)

@lforst
Copy link
Member

lforst commented Apr 4, 2023

But that sounds more or less like what I'm doing - picking and choosing specific exports to expose. Just in a slightly different way 🙂.

No not really. It's either export * from '@sentry/node' or export * from '@sentry/browser' depending on whether we're in browser or node land. We're not picking and choosing particular exports.

@rchl
Copy link
Contributor Author

rchl commented Apr 4, 2023

But that sounds more or less like what I'm doing - picking and choosing specific exports to expose. Just in a slightly different way 🙂.

No not really. It's either export * from '@sentry/node' or export * from '@sentry/browser' depending on whether we're in browser or node land. We're not picking and choosing particular exports.

That actually sounds like a good idea to expose a meta import like that that. It would be a breaking change for the Nuxt module but it sounds like it should solve the main issues.

If the user would want to import a browser-only export like showReportDialog then I guess it might still require a bit of gymnastics with async import but it might not be a big issue. Or maybe there could be shims provided so that on the server side it would import some dummy shim instead. Do you do something like that for Next?

@lforst
Copy link
Member

lforst commented Apr 5, 2023

If the user would want to import a browser-only export like showReportDialog then I guess it might still require a bit of gymnastics with async import but it might not be a big issue. Or maybe there could be shims provided so that on the server side it would import some dummy shim instead. Do you do something like that for Next?

Generally, we assume users are smart enough not to use any browser features in their server code. If they would however do so, the error messages are usually quite transparent so people will figure out quickly what's going on.

The only situation where we need to shim is when Next.js interprets a file as both server and client code. We know of a few (server) methods of ours that will be used in these files so we make their client counterpart a no-op.

So far I think this is working fine. At first, it was a bit tricky to get it working for everybody but by now bug reports have calmed down to almost 0. I think it is a good trade-off between UX and functionality. Of course, it is not the most "correct" solution, but that is usually not what we strive for.

@Lonli-Lokli
Copy link

Lonli-Lokli commented Apr 10, 2023

I've tried to compare the minimal size of the bundle with the sentry, and it appears a lot of.
This snippet adds 76Kb of size

import { init, createErrorHandler } from '@sentry/angular-ivy';

init({
  dsn: 'https://<key>@sentry.io/<project>',
});

@NgModule({
  declarations: [AppComponent],
  imports: [BrowserModule, AppRoutingModule],
  providers: [
    {
      provide: ErrorHandler,
      useValue: createErrorHandler({
        showDialog: true,
      }),
    },
  ],
  bootstrap: [AppComponent],
})
export class AppModule {}

EDIT: with this modifications size has been decreased to 70.4Kb

module.exports = {
  // ... other options
  plugins: [
    new webpack.DefinePlugin({
      __SENTRY_DEBUG__: false,
      __SENTRY_TRACING__: false,
    })
  ],
};

init({
  dsn: 'https://<key>@sentry.io/<project>',
  defaultIntegrations: []
});

@rchl
Copy link
Contributor Author

rchl commented Apr 14, 2023

Was decided that no action needs to be taken.

@rchl rchl closed this as completed Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants