ref: Optimize build configs and builds #480
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
The build changes look fine to me although I remember we had some issues with process for some reason ending up in client code and of course being undefined. I'm curious if the changes here solve this or if we'd regress. But to be honest, I didn't make these changes back then so can't tell you specifics unfortunately :/
Also had another question
| integrations: [customIntegration()], | ||
| }); | ||
| } | ||
| Spotlight.init({ |
There was a problem hiding this comment.
I'm aware that this is just an e2e test app but is this how we want users to use Spotlight in the future?
I'm probably missing something but if this guard is removed, spotlight overlay code ends up in the production build of the NextJS application. Currently we document spotlight setup with this guard in place so that users don't accidentally include it in their prod bundles.
The reason is that bundlers are smart enough to remove code within the process.env.NODE_ENV check if the condition evaluates to false at build time.
| @@ -12,5 +12,5 @@ Sentry.init({ | |||
|
|
|||
| // Setting this option to true will print useful information to the console while you're setting up Sentry. | |||
| debug: false, | |||
| spotlight: process.env.NODE_ENV === 'development', | |||
| spotlight: true, | |||
There was a problem hiding this comment.
same question generally here: If this is always true, the integration will also try to send events to the side car in production. Is this what we want?
There was a problem hiding this comment.
I never considered these test codes as representative examples for usage (as we also set debug: true on some places). I removed the conditionals mostly to avoid any confusing test issues in case we mess with NODE_ENV value for some reason (builds, installing dependencies etc).
For representative code examples, docs should be the main source of truth and yes ideally, Spotlight should not be included in production code but if one wants to do it, we do have a warning somewhere in the code :)
There was a problem hiding this comment.
Ok I see, that makes sense! Fwiw, at least when I worked on Spotlight the e2e tests were quite messy and probably didn't always reflect what we instruct users to do. I think generally, it's good to stay close to how we instruct the setup but honestly, no strong feelings for this one here.
Just wanted to make sure we're aware of the bundling/tree-shaking checks around prod vs dev.
For this, I rely on e2e tests as it should be caught there. That said I'll do another manual pass before merging to make sure this doesn't cause any issues. It may as well be solved with newer versions of |
This change mainly removes the enforcement of
process.env.NODE_ENV === 'development'for Spotlight which in turn also enables significant bundle size savings as we include the React library which does optimizations based on this value.Without the patch:
With the patch: