-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(browser): Include Spotlight in development bundles #18078
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
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
Lms24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
8d0f261 to
92bc9f6
Compare
size-limit report 📦
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah probably best in a follow up PR. If we can keep it between the magic comments it'll all get striped out!
…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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Lms24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for invstigating and implementing this Tim! I think this is a very reasonable solution.
|
Hi, This is a breaking change, yet it was brought in a minor version increment, thus breaking my apps having the following URL in their "@sentry/browser": "https://cdn.jsdelivr.net/npm/@sentry/browser@10/build/npm/esm/index.js/+esm"Because Please revert. |
|
Hey @KaKi87 thanks for reporting! Apologies that this change caused your app to break! That was definitely not our intention. However, in this case, I don't believe that our package-internal paths should be considered public API. Our package.json exports are of course, so we wouldn't remove or rename them without a new major version. I also want to note that we don't officially support jsdelivr, unpkg or other 3rd party CDN providers. We host our own set of CDN bundles as well as a lazy loader which of course have the same semver guarantees as our NPM packages. These bundles are not affected by this PR. Also, depending on your use case, our bundles are quite a bit smaller than the jsdelivr ones because we make extensive use of tree shaking when building them. Just a thought :) In case you want to continue using jsdelivr, I highly recommend using a URL that doesn't rely on an internal package structure. As far as I can tell, jsdelivr also recommends a URL without any subpaths: Again, sorry for breaking you. I will keep looking at this PR and the comments to see how many people were affected. We can re-evaluate next steps in case other problems come up. For now though, we'll keep this PR as-is. |
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