Skip to content

build(utils): Add build constant for cdn vs. npm bundles - #6904

Merged
AbhiPrasad merged 4 commits into
masterfrom
abhi-build-const-is-cdn
Jan 31, 2023
Merged

build(utils): Add build constant for cdn vs. npm bundles#6904
AbhiPrasad merged 4 commits into
masterfrom
abhi-build-const-is-cdn

Conversation

@AbhiPrasad

@AbhiPrasadAbhiPrasad commented Jan 23, 2023

Copy link
Copy Markdown
Contributor

ref: #6903

Add __SENTRY_SDK_SOURCE__ that tracks the source of the sdk. Right now there are 3 specific values for this: 'npm' | 'cdn' | 'loader'.

npm and cdn are filled in at build time, and loader will be added via a global flag that the loader itself will set. More details on this soon.

Comment threadrollup/bundleHelpers.js Outdated
entryFileNames: chunkInfo => `${baseConfig.output.entryFileNames(chunkInfo)}.min.js`,
},
plugins: [stripDebuggingPlugin, terserPlugin],
plugins: [stripDebuggingPlugin, terserPlugin, isCDNBundlePlugin],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should probably do this before terser minifies

Comment threadrollup/plugins/bundlePlugins.js Outdated
export function makeIsCDNBundlePlugin(isCDNBundle) {
return replace({
values: {
__SENTRY_CDN_BUNDLE__: isCDNBundle,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could think about making it more explicit that this takes a string but I don't care too much: isCDNBundle ? 'true' : 'false'

Comment threadrollup/plugins/bundlePlugins.js Outdated
export function makeIsCDNBundlePlugin(isCDNBundle) {
return replace({
values: {
__SENTRY_CDN_BUNDLE__: isCDNBundle,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IIRC the replace plugin wants us to set some preventAssignment option. I would set it to false here.

@Lms24Lms24 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should go with a non-boolean approach here. For instance, if we ever wanted to track loader vs regular CDN usage, we'd need to make some changes here. Not sure if I'm missing something but couldn't we just use the rollup replace plugin to replace something like __SDK_SOURCE__ with string literals ('npm' | 'cdn' | 'loader')?

@AbhiPrasad
AbhiPrasadforce-pushed the abhi-build-const-is-cdn branch from adaee78 to e18ec72CompareJanuary 30, 2023 12:17
@AbhiPrasad
AbhiPrasad marked this pull request as ready for review January 30, 2023 12:21

@Lms24Lms24 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for addressing my feedback!

@AbhiPrasad
AbhiPrasad enabled auto-merge (squash) January 31, 2023 10:34
@AbhiPrasadAbhiPrasad self-assigned this Jan 31, 2023
@AbhiPrasad
AbhiPrasad merged commit 8c350e7 into masterJan 31, 2023
@AbhiPrasad
AbhiPrasad deleted the abhi-build-const-is-cdn branch January 31, 2023 10:36
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@AbhiPrasad@lforst@Lms24