fix(build): Prevent Node's `Buffer` module from being included in browser bundles #3372

lobsterkatie · 2021-04-07T00:23:27Z

NB: After it was opened, this PR was totally overhauled and simplified considerably. The original PR description is included below so that the review comments make any sense at all, but to be clear, none of it applies to the current implementation.

Prior to this PR, the fact that the Node Buffer class made a (naked) appearance in @sentry/utils meant that webpack included it browser bundles*, noticeably increasing their size. This fixes that by changing all Buffer references in @sentry/utils (all two of them!) from Buffer to globalObject.Buffer, which it turns out is enough to convince webpack not to include it. (Who knew?) It does a similar thing for atob and btoa in the browser, just for consistency. Other changes include:

reorganizing error handling
adding typechecking
porting tests to @sentry/browser since the implementation of the base64 functions is different in browser than in node (otherwise none of this would be a problem!)

*To be clear, in this case "browser bundles" means the bundles a SDK user would create were they using Sentry in their web app, not the bundles we serve from our CDN. Our bundles, because they're created with Rollup and not webpack, were already fine.

Original description (outdated):

This PR pulls the code which uses Buffer back into @sentry/node (and does a similar thing for atob and btoa in the browser), and refactors to use it from there. Specifically, it stashes the relevant functions on window.__SENTRY__.extensions or global.__SENTRY__.extensions on SDK startup, and then @sentry/utils grabs them from there.

Changes necessitated by the above:

Move Carrier interface and getMainCarrirer function out of @sentry/hub, into (respectively) @sentry/types and @sentry/utils. Both are still exported from @sentry/hub for backwards compatibility.
Specify a dom testing environment where appropriate.
Stop testing @sentry/tracing against Node 6.

github-actions · 2021-04-07T00:36:16Z

size-limit report

Path	Size
@sentry/browser - CDN Bundle (gzipped)	20.8 KB (-30.54% 🔽)
@sentry/browser - Webpack	21.63 KB (-29.7% 🔽)
@sentry/react - Webpack	21.67 KB (-29.64% 🔽)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped)	28.76 KB (-21.8% 🔽)

rhcarvalho

I suggest renaming packages/node/src/string.ts -> packages/node/src/base64.ts since the contents there are only related to base64 conversion (and them end up with files with the same name in browser and node for ease of discoverability and navigating the code base)

rhcarvalho · 2021-04-08T09:46:04Z

packages/browser/src/helpers.ts

+ * (This is here (and in the node package) rather than in @sentry/utils to prevent webpack from bundling the entire
+ * Buffer module when creating SDK users' vendor bundles for the browser. Used in @sentry/utils.string.)


I'd avoid making certain cross-references that can get outdated. Comments are not good for that, and we have tools to figure out where a certain piece of code is used.
I'd also avoid putting such comments in the jsdoc of functions, since they are unrelated to how the function works and would end up polluting the generated documentation.

My suggestion is to have all related functions in a single file and have a single comment applying to the whole file.

File: packages/browser/src/base64.ts

// This file is here instead of in @sentry/utils such that users of the Browser SDK using webpack // do not have Node's Buffer module unnecessarily included in their bundles. // ...

I think you're right about not polluting the JSDoc with it, but I kept a similar comment because, sure, you can search for something across packages, but who thinks to do that if you're just changing one test for whatever reason? This way, anyone changing things in one place knows immediately that it needs to be done elsewhere as well (even if that change is taking them out of sync for some reason, in which case they'll know to delete the comment in both places!).

rhcarvalho · 2021-04-08T09:49:51Z

I'm really puzzled by the bundle size gains in #3372 (comment), I wonder if we can trust that since this PR is not targeting master. What is the size comparison comparing?

lobsterkatie · 2021-04-08T19:24:34Z

I'm really puzzled by the bundle size gains in #3372 (comment), I wonder if we can trust that since this PR is not targeting master. What is the size comparison comparing?

It's comparing to the branch, I believe. The whole reason for this PR is that this branch's changes had caused the package size to go way up; this brings it back down to baseline by fixing the cause of the increase. That's why you're seeing such a big percentage drop. (I tested it locally, and the numbers in the comment are correct.)

packages/utils/src/string.ts

…wser bundles (#3372)

This is the result of rebasing the feature branch for the initial implementation of `tracestate` header handling (which had grown very stale) on top of current `master`. That branch is going to get deleted, so for posterity, it included the following PRs (oldest -> newest): feat(tracing): Add dynamic sampling correlation context data to envelope header (#3062) chore(utils): Split browser/node compatibility utils into separate module (#3123) ref(tracing): Prework for initial `tracestate` implementation (#3242) feat(tracing): Add `tracestate` header to outgoing requests (#3092) ref(tracing): Rework tracestate internals to allow for third-party data (#3266) feat(tracing): Handle incoming tracestate data, allow for third-party data (#3275) chore(tracing): Various small fixes to first `tracestate` implementation (#3291) fix(tracing): Use `Request.headers.append` correctly (#3311) feat(tracing): Add user data to tracestate header (#3343) chore(various): Small fixes (#3368) fix(build): Prevent Node's `Buffer` module from being included in browser bundles (#3372) fix(tracing): Remove undefined tracestate data rather than setting it to `null` (#3373) More detail in the PR description.

lobsterkatie requested a review from kamilogorek as a code owner April 7, 2021 00:23

rhcarvalho suggested changes Apr 8, 2021

View reviewed changes

lobsterkatie force-pushed the kmclb-stop-bundling-buffer-module branch from ba918c3 to 00cfee2 Compare April 8, 2021 18:42

lobsterkatie added 4 commits April 8, 2021 12:04

consolidate error handling

512387b

get Buffer and atob/btoa from global object explicitly

323a275

add typechecking

14636e8

test in browser as well as node

9228f1e

lobsterkatie force-pushed the kmclb-stop-bundling-buffer-module branch from 00cfee2 to 9228f1e Compare April 8, 2021 19:07

lobsterkatie requested a review from rhcarvalho April 8, 2021 19:25

kamilogorek suggested changes Apr 9, 2021

View reviewed changes

packages/utils/src/string.ts Outdated Show resolved Hide resolved

packages/utils/src/string.ts Outdated Show resolved Hide resolved

packages/utils/src/string.ts Outdated Show resolved Hide resolved

packages/utils/src/string.ts Outdated Show resolved Hide resolved

kamilogorek reviewed Apr 10, 2021

View reviewed changes

packages/utils/src/string.ts Show resolved Hide resolved

minor fixes

a957d9d

lobsterkatie force-pushed the kmclb-stop-bundling-buffer-module branch from 40f66c4 to a957d9d Compare April 12, 2021 17:05

lobsterkatie requested a review from kamilogorek April 12, 2021 17:29

kamilogorek approved these changes Apr 12, 2021

View reviewed changes

lobsterkatie merged commit 271f7ec into kmclb-dynamic-sampling-data-in-envelopes Apr 12, 2021

lobsterkatie deleted the kmclb-stop-bundling-buffer-module branch April 12, 2021 18:56

lobsterkatie added a commit that referenced this pull request Apr 12, 2021

fix(build): Prevent Node's Buffer module from being included in bro…

1762574

…wser bundles (#3372)

lobsterkatie mentioned this pull request Aug 20, 2021

feat(tracing): Add initial tracestate header handling #3909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(build): Prevent Node's `Buffer` module from being included in browser bundles #3372

fix(build): Prevent Node's `Buffer` module from being included in browser bundles #3372

Uh oh!

lobsterkatie commented Apr 7, 2021 •

edited

Loading

Uh oh!

github-actions bot commented Apr 7, 2021 •

edited

Loading

Uh oh!

rhcarvalho left a comment

Uh oh!

rhcarvalho Apr 8, 2021

Uh oh!

lobsterkatie Apr 8, 2021

Uh oh!

rhcarvalho commented Apr 8, 2021

Uh oh!

lobsterkatie commented Apr 8, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		* (This is here (and in the node package) rather than in @sentry/utils to prevent webpack from bundling the entire
		* Buffer module when creating SDK users' vendor bundles for the browser. Used in @sentry/utils.string.)

Uh oh!

fix(build): Prevent Node's Buffer module from being included in browser bundles #3372

fix(build): Prevent Node's Buffer module from being included in browser bundles #3372

Uh oh!

Conversation

lobsterkatie commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report

Uh oh!

rhcarvalho left a comment

Choose a reason for hiding this comment

Uh oh!

rhcarvalho Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

lobsterkatie Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

rhcarvalho commented Apr 8, 2021

Uh oh!

lobsterkatie commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fix(build): Prevent Node's `Buffer` module from being included in browser bundles #3372

fix(build): Prevent Node's `Buffer` module from being included in browser bundles #3372

lobsterkatie commented Apr 7, 2021 •

edited

Loading

github-actions bot commented Apr 7, 2021 •

edited

Loading

lobsterkatie commented Apr 8, 2021 •

edited

Loading