-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(build): Vendor polyfills injected during build #5051
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8794b61
to
af7fdbe
Compare
size-limit report 📦
|
af7fdbe
to
54f33b6
Compare
54f33b6
to
71e8b13
Compare
Lms24
approved these changes
May 9, 2022
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 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.
71e8b13
to
bba1ef5
Compare
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
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 withtslib
, however, there's no option equivalent to tsc'simportHelpers
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 ofimportHelpers
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:
@sentry/utils
, in a separate folder undersrc/
. 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 inutils
, 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 inutils
was the simplest and most logical option.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.[ 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 genericunknown
. 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 useArray.prototype.flat
in tests run on older versions of Node, it itself has been polyfilled, and our testingtsconfig
target ramped back down when running tests under those old versions.