Skip to content
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

feat(build): Vendor polyfills injected during build #5051

Merged
merged 9 commits into from
May 9, 2022

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented May 9, 2022

Both Rollup and Sucrase add polyfills (similar to those in tslib) during the build process, to down-compile newer language features and help ESM and CJS modules play nicely together. Unlike with tslib, however, there's no option equivalent to tsc's importHelpers option, which allows those polyfills to be imports from one central place rather than copies of the full code for each necessary polyfill in every file that uses them. Having all that repeated code is a surefire way to wreck our bundle-size progress, so as part of the switch to using those tools, we will replicate the behavior of importHelpers in the context of our new sucrase/rollup builds.

This is the first of two PRs accomplishing that change. In this one, the polyfills we'll use have been added to the repo. In the next one, a rollup plugin will be added to replace the duplicated injected code with import statements, which will then import the polyfills this PR adds. Originally this was one PR, but it has been split into two for easier reviewing.

Notes:

  • The polyfills themselves have been added to @sentry/utils, in a separate folder under src/. Various discussions have been had about where they should live, and they have moved around as this PR evolved. Without going into too much boring detail about about the exact pros and cons of each location, suffice it to say that my first instinct was to put them in utils, and as I worked on other options, each one of the reasons why I didn't actually do that to begin with (they weren't in TS, they weren't originally ours, I didn't want to pollute the @sentry/utils namespace) got solved, and in the end, putting them in utils was the simplest and most logical option.
  • The added polyfills are copies of the code injected by Sucrase or Rollup, translated into TS and in some cases modified in order to make them less verbose. Since some treeshaking algorithms can only work file by file, each one is in its own file. Polyfills which were modified have had tests added, both to ensure that they do the same thing as the originals, and to ensure that what they do is actually correct.
  • As we're not the original authors, attribution has been given within each file to the original source. Further, since the MIT license under which both Rollup and Sucrase are distributed requires duplication of the license blurb, it has been added to the README in the polyfills' folder.
  • Because we don't want these to become part of our public API, their code is not exported by index.ts, meaning they are not importable directly from @sentry/utils and won't come up under intellisense code completions and the like. When our code imports them, it will import from @sentry/utils/cjs/buildPolyfills or @sentry/utils/esm/buildPolyfills as appropriate.
  • Though not every polyfill Rollup and Sucrase can inject ends up being used in our built code, they are all included just to cover our bases. That said, in the interest of time, only the ones we use or are likely to use have had tests written for them.
  • One of the polyfills we do use is the optional chain polyfill. Typing it is tricky, because it takes a variable number of arguments, whose types are effectively [ A, B, C, B, C, B, C,... ] (one type for the first argument, and then alternating types for the rest). Because it's not going to be used by the public, the typing in the polyfill itself uses the generic unknown. For tests it was easier, because only the test cases needed to be typed, and could have a slightly modified structure: [ A, [ B, C ][] ], which then gets flattened again before being passed to the optional chain polyfill. In order to be able to use Array.prototype.flat in tests run on older versions of Node, it itself has been polyfilled, and our testing tsconfig target ramped back down when running tests under those old versions.

@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 18.76 KB (-6.87% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 58.58 KB (-9.35% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 17.55 KB (-6.97% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 52.76 KB (-9% 🔽)
@sentry/browser - Webpack (gzipped + minified) 19.21 KB (-17.35% 🔽)
@sentry/browser - Webpack (minified) 62.12 KB (-23.99% 🔽)
@sentry/react - Webpack (gzipped + minified) 19.23 KB (-17.39% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified) 42.75 KB (-11.04% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 24.42 KB (-6.34% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 23 KB (-6.08% 🔽)

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation the PR description. I think we've gone back and forth about it and if putting these files into utils is the best solution, I'm totally okay with it.

@lobsterkatie lobsterkatie merged commit 19a9d00 into 7.x May 9, 2022
@lobsterkatie lobsterkatie deleted the kmclb-add-build-polyfills branch May 9, 2022 17:00
lobsterkatie added a commit that referenced this pull request May 10, 2022
This is a follow-up to #5051, which added our own versions of the polyfills that Rollup and Sucrase inject during build, in order to be able to import them rather than have their code repeated in every file in which they're needed, essentially replicating the behavior of `tslib`'s `importHelpers` option. This completes that change, by adding a rollup plugin to the build process which extracts the injected code and replaces it with an equivalent `import` or `require` statement. Because the import comes from `@sentry/utils`, the few packages which didn't already depend on it have had it added as a dependency.
@AbhiPrasad AbhiPrasad added this to the 7.0.0 milestone May 12, 2022
AbhiPrasad pushed a commit that referenced this pull request May 30, 2022
Both Rollup and Sucrase add polyfills (similar to those in `tslib`) during the build process, to down-compile newer language features and help ESM and CJS modules play nicely together. Unlike with `tslib`, however, there's no option equivalent to tsc's `importHelpers` option[1], which allows those polyfills to be imports from one central place rather than copies of the full code for each necessary polyfill in every file that uses them. Having all that repeated code is a surefire way to wreck our bundle-size progress, so as part of the switch to using those tools, we will replicate the behavior of `importHelpers` in the context of our new sucrase/rollup builds.

This is the first of two PRs accomplishing that change. In this one, the polyfills we'll use have been added to the repo. In the next one, a rollup plugin will be added to replace the duplicated injected code with import statements, which will then import the polyfills this PR adds. Originally this was one PR, but it has been split into two for easier reviewing.

Notes:

- The polyfills themselves have been added to `@sentry/utils`, in a separate folder under `src/`. Various discussions have been had about where they should live[2], and they have moved around as this PR evolved. Without going into too much boring detail about about the exact pros and cons of each location, suffice it to say that my first instinct was to put them in `utils`, and as I worked on other options, each one of the reasons why I didn't actually do that to begin with (they weren't in TS, they weren't originally ours, I didn't want to pollute the `@sentry/utils` namespace) got solved, and in the end, putting them in `utils` was the simplest and most logical option.
- The added polyfills are copies of the code injected by Sucrase or Rollup, translated into TS and in some cases modified in order to make them less verbose. Since some treeshaking algorithms can only work file by file, each one is in its own file. Polyfills which were modified have had tests added, both to ensure that they do the same thing as the originals, and to ensure that what they do is actually correct.
- As we're not the original authors, attribution has been given within each file to the original source. Further, since the MIT license under which both Rollup and Sucrase are distributed requires duplication of the license blurb, it has been added to the README in the polyfills' folder.
- Because we don't want these to become part of our public API, their code is not exported by `index.ts`, meaning they are not importable directly from `@sentry/utils` and won't come up under intellisense code completions and the like. When our code imports them, it will import from `@sentry/utils/cjs/buildPolyfills` or `@sentry/utils/esm/buildPolyfills` as appropriate.
- Though not every polyfill Rollup and Sucrase can inject ends up being used in our built code, they are all included just to cover our bases. That said, in the interest of time, only the ones we use or are likely to use have had tests written for them. 
- One of the polyfills we do use is the optional chain polyfill. Typing it is tricky, because it takes a variable number of arguments, whose types are effectively `[ A, B, C, B, C, B, C,... ]` (one type for the first argument, and then alternating types for the rest). Because it's not going to be used by the public, the typing in the polyfill itself uses the generic `unknown`. For tests it was easier, because only the test cases needed to be typed, and could have a slightly modified structure: `[ A, [ B, C ][] ]`, which then gets flattened again before being passed to the optional chain polyfill. In order to be able to use `Array.prototype.flat` in tests run on older versions of Node, it itself has been polyfilled, and our testing `tsconfig` target ramped back down when running tests under those old versions.

[1] https://www.typescriptlang.org/tsconfig#importHelpers
[2] #5023 (comment)
AbhiPrasad pushed a commit that referenced this pull request May 30, 2022
This is a follow-up to #5051, which added our own versions of the polyfills that Rollup and Sucrase inject during build, in order to be able to import them rather than have their code repeated in every file in which they're needed, essentially replicating the behavior of `tslib`'s `importHelpers` option. This completes that change, by adding a rollup plugin to the build process which extracts the injected code and replaces it with an equivalent `import` or `require` statement. Because the import comes from `@sentry/utils`, the few packages which didn't already depend on it have had it added as a dependency.
Sign up for free to 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.

None yet

3 participants