feat(browser): Include Spotlight in development bundles #18078
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
This looks really promising, thanks Tim!
(I realize this is still in draft so feel free to ignore this review if you're still planning on making changes)
timfish
force-pushed
the
timfish/build/dev-prod-browser-build
branch
from
8d0f261 to
92bc9f6
Compare
size-limit report 📦
|
timfish
force-pushed
the
timfish/build/dev-prod-browser-build
branch
from
41be108 to
eb515c4
Compare
|
Needs some tests to confirm Spotlight actually works in dev mode... |
| options.defaultIntegrations == null ? getDefaultIntegrations(options) : options.defaultIntegrations; | ||
|
|
||
| /* rollup-include-development-only */ | ||
| if (options.spotlight) { |
There was a problem hiding this comment.
How hard would it be to fill this value from the env variables? I'm okay with trying all variants like PUBLIC_SENTRY_SPOTLIGHT or VITE_SENTRY_SPOTLIGHT etc.
Happy to do this in a follow up too.
There was a problem hiding this comment.
Yeah probably best in a follow up PR. If we can keep it between the magic comments it'll all get striped out!
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
timfish
changed the title
…sentry/sentry-javascript into timfish/build/dev-prod-browser-build
…sentry/sentry-javascript into timfish/build/dev-prod-browser-build
|
|
||
| function readAllJs(outDir: string): string { | ||
| let contents = ''; | ||
| const stack = [outDir]; |
There was a problem hiding this comment.
There's a simpler and probably more efficient way now: https://github.com/getsentry/sentry-docs/pull/13994/files#diff-a17104741f091f6ca105b7bd4f8272c206b4575306615ebac8a847cc004591c3R45-R50
| } | ||
|
|
||
| export function makeProductionReplacePlugin() { | ||
| const pattern = /\/\* rollup-include-development-only \*\/[\s\S]*?\/\* rollup-include-development-only-end \*\/\s*/g; |
There was a problem hiding this comment.
Not very confident about using regex here. I'm guessing AST based operation is either unavailable or more expensive?
There was a problem hiding this comment.
Are you okay me exploring rollup-plugin-define or Vite (which uses rolldown under the hood which should be fully compatible with rollup) that has the define feature built-in: https://vite.dev/config/#using-environment-variables-in-config
Once we do that, we can set NODE_ENV to development or production based on the build, and use regular if blocks rather than surround the code with magic comments. These if blocks will have a static condition at build time which will then be optimized (stripped out or unwrapped) via terser or whatever minified/optimizer we are using.
There was a problem hiding this comment.
When building the SDK npm module, we preserve the individual files (ie. preserveModules: true) and we don't do any kind of minification or optimization.
This is why I went this route vs replace/define plugins.
This PR:
Vite and Webpack will automatically include Spotlight in development/production mode and it is excluded in production. Unfortunately, Rollup will not select
developmentunless you set theexportConditionsfor@rollup/plugin-node-resolve