feat(build): Add polyfill-extraction rollup plugin #5023
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
lobsterkatie
force-pushed
the
kmclb-add-polyfill-extraction-rollup-plugin
branch
3 times, most recently
from
f02192e to
ce0f043
Compare
size-limit report 📦
|
lobsterkatie
force-pushed
the
kmclb-add-polyfill-extraction-rollup-plugin
branch
2 times, most recently
from
d3a844a to
ad4ce22
Compare
|
Does this mean we can remove |
lobsterkatie
force-pushed
the
kmclb-add-polyfill-extraction-rollup-plugin
branch
from
ad4ce22 to
f10ba18
Compare
Once we switch over, yes, that's the idea. We're basically making our own |
AbhiPrasad
reviewed
May 3, 2022
| const importSource = literal( | ||
| isUtilsPackage | ||
| ? path.relative(path.dirname(currentSourceFile), `../jsPolyfills/${moduleFormat}`) | ||
| : `@sentry/utils/jsPolyfills/${moduleFormat}`, |
There was a problem hiding this comment.
should we make another package for the polyfills?
There was a problem hiding this comment.
In the long term, they might well live in a
@sentry/dev-utils/@sentry/internal-utils/@sentry/whatever package, but to get things running, for now the rollup directory seemed like a good place
In the long run I agree that'd be a better place, but I'd say it's okay to leave this as an iterative improvement. What do you think?
There was a problem hiding this comment.
I'm +1 on an extra package but it's probably fine if we do it in a follow-up task.
There was a problem hiding this comment.
I don't think they warrant a package all on their own, but I did experiment with moving them to a dev-utils package. Once I got them there, though, I didn't love that it was then going to become a runtime dependency of @sentry/utils, when it really should only be a dev dependency. And as part of that experimentation, I did end up translating them to TS, which was the final remaining blocker to them living directly in @sentry/utils, so that is where they ended up.
(The other two original blockers for me were:
- Feeling weird about putting code that wasn't ours in with code that was. This was solved for me by 1) the fact that we now go overboard in terms of attribution, license-copying, etc, and 2) the fact that in fact they aren't exact copies of the originals anymore, if for no other reason than the translation to TS.
- Not wanting them to be part of the public API of
@sentry/utils. Originally that meant that I was copying them over to a folder outside ofbuild, but I realized that their built versions actually can live insidebuildand still be "hidden" (in that they won't show up in intellisense completions, for example) as long as they're not exported fromindex.ts. That then let me put the folder containing the pre-build versions straight intosrc/, which made everything about building and testing them a lot easier.)
lobsterkatie
force-pushed
the
kmclb-add-polyfill-extraction-rollup-plugin
branch
from
f10ba18 to
1f2dee5
Compare
Lms24
reviewed
May 4, 2022
There was a problem hiding this comment.
So I took a look at the PR and overall this looks good to me with the asterisk that I'm not entirely comfortable with having our "own" polyfills because to me this seems kind of strange. But then again, I clearly don't have as much context here as you so please take it with a grain of salt.
I do have a few questions though:
- Just to double check - there isn't a way around having our own polyfills, right? I was thinking of something like importing them from sucrase/rollup/tslib instead of adding them to our repo where we have to maintain them. In my mind it would be best to just import them and add the
importHelpersfunctionality around them. But again, I probably lack a lot of necessary context here. - The polyfills we're adding to our repo in this PR - are they exact copies of the Sucrase/Rollup polyfilles?
- If yes, do we need to acknowledge this in the code files/give credits, etc?
- if no, I really think we should have tests for them (would be best either way, I guess)
Anyway, great work and thanks for figuring all of this out!
Believe me, I wish I saw a way around it, because I agree, I'd rather leave maintenance/testing/etc to them instead of having to do it ourselves. But it's unfortunately not as simple as importing the helpers, because they don't exist in a straightforwardly importable form. Sucrase uses a
In the case of everything other than the iterop helpers, yes, they're the same as what's emitted by sucrase (the only ones emitted by rollup are the interop ones). As far as the interop ones go, here are the originals (we use either the
From the first line of the README distributed with the polyfills: "This is a collection of syntax polyfills either copied directly from or heavily inspired by those used by Rollup and Sucrase." What do you think, is more necessary?
For those interop ones, I'm happy to work on tests now if it would up our overall confidence level in making this change. I'll update here once I've added some. UPDATE: The tests have indeed been added, but they increased the size of this PR sufficiently that I thought it would be easier to review if I split it in two. They can be found in #5051. |
lobsterkatie
force-pushed
the
kmclb-add-polyfill-extraction-rollup-plugin
branch
7 times, most recently
from
a3283e3 to
2d66612
Compare
lobsterkatie
force-pushed
the
kmclb-add-polyfill-extraction-rollup-plugin
branch
from
2d66612 to
b5bf453
Compare
lobsterkatie
added a commit
that referenced
this pull request
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[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)
lobsterkatie
force-pushed
the
kmclb-add-polyfill-extraction-rollup-plugin
branch
from
b5bf453 to
abce946
Compare
Lms24
approved these changes
May 10, 2022
lobsterkatie
added a commit
that referenced
this pull request
May 10, 2022
This switches our `build` and `build:dev` yarn scripts to use the new rollup/sucrase build process. This is the culmination of a number of previous changes, whose highlights include: - #4724, which made building types files a separate build process - #4895, which updated the SDK to use TS 3.8.3 - #4926, which removed use of the `Severity` enum - #5005, which switch our default tsconfig to target es6 - #4992, which added the Sucrase plugin, some helper functions, and the `yarn build:rollup` script - #4993, which added rollup plugins to use `var` rather than `const` and clean up the built code in various ways - #5022, which applied the same `const`-to-`var` translation to tests - #5023, which added the ability to change injected polyfills into imports The result is that, as of this PR, we will no longer use `tsc` to transpile or down-complile our code when building npm packages. Instead, we will be using Rollup to handle making our code CJS-friendlly and Sucrase to handle the transpilation from TS to JS. The main advantages of this change are: - It forced us to do a lot of updating, centralizing, and cleanup of our tooling, not just for build but also for testing and linting. - It brings our CDN build process and our npm build process much more in line with each other, for easier maintainability. - It gives us more control over the eventual output, because we have access to a whole internet full of Rollup plugins (not to mention the ability to make our own), rather than being constrained to tsconfig options. (Plugins also allow us to interact with the code directly.) - It speeds up our builds fairly significantly. I ran a number of trials in GHA of running `yarn build:dev` at the top level of the repo. Before this change, the average time was ~150 seconds. After this change, it's about half that, roughly 75 seconds. Because of the switch in tooling, the code we publish is going to be slightly different. In order to make sure that those differences weren't going to be breaking, I built each package under the old system and under the new system, ran a `git diff`, and checked every file, both CJS and ESM, in every package affected by this change. The differences (none of which affect behavior or eventual bundle size by more than a few bytes in each direction), fell into a few categories: - Purely cosmetic changes, things like which comments are retained, the order of imports, where in the file exports live, etc. - Changes to class constructors, things like not explicitly assigning `undefined` to undefined attributes, using regular assignment rather than `Object.defineProperty` for attributes which are assigned values, and splitting some of those assignments off into helper functions. - Changes related to the upgrade to ES6 and dropping of support for Node 6, things like not polyfilling object spread or async/await While this represents the most significant part of the overall change, a few outstanding tasks remain: - Making this same switch in `build:watch` - Parallelizing the builds, both locally and in CI - Perhaps applying the new process to our CDN bundle builds - Generalized cleanup These will all be included in separate PRs, some in the immediate future and some in the hopefully-not-too-distant short-to-medium term.
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.
AbhiPrasad
pushed a commit
that referenced
this pull request
May 30, 2022
This switches our `build` and `build:dev` yarn scripts to use the new rollup/sucrase build process. This is the culmination of a number of previous changes, whose highlights include: - #4724, which made building types files a separate build process - #4895, which updated the SDK to use TS 3.8.3 - #4926, which removed use of the `Severity` enum - #5005, which switch our default tsconfig to target es6 - #4992, which added the Sucrase plugin, some helper functions, and the `yarn build:rollup` script - #4993, which added rollup plugins to use `var` rather than `const` and clean up the built code in various ways - #5022, which applied the same `const`-to-`var` translation to tests - #5023, which added the ability to change injected polyfills into imports The result is that, as of this PR, we will no longer use `tsc` to transpile or down-complile our code when building npm packages. Instead, we will be using Rollup to handle making our code CJS-friendlly and Sucrase to handle the transpilation from TS to JS. The main advantages of this change are: - It forced us to do a lot of updating, centralizing, and cleanup of our tooling, not just for build but also for testing and linting. - It brings our CDN build process and our npm build process much more in line with each other, for easier maintainability. - It gives us more control over the eventual output, because we have access to a whole internet full of Rollup plugins (not to mention the ability to make our own), rather than being constrained to tsconfig options. (Plugins also allow us to interact with the code directly.) - It speeds up our builds fairly significantly. I ran a number of trials in GHA of running `yarn build:dev` at the top level of the repo. Before this change, the average time was ~150 seconds. After this change, it's about half that, roughly 75 seconds. Because of the switch in tooling, the code we publish is going to be slightly different. In order to make sure that those differences weren't going to be breaking, I built each package under the old system and under the new system, ran a `git diff`, and checked every file, both CJS and ESM, in every package affected by this change. The differences (none of which affect behavior or eventual bundle size by more than a few bytes in each direction), fell into a few categories: - Purely cosmetic changes, things like which comments are retained, the order of imports, where in the file exports live, etc. - Changes to class constructors, things like not explicitly assigning `undefined` to undefined attributes, using regular assignment rather than `Object.defineProperty` for attributes which are assigned values, and splitting some of those assignments off into helper functions. - Changes related to the upgrade to ES6 and dropping of support for Node 6, things like not polyfilling object spread or async/await While this represents the most significant part of the overall change, a few outstanding tasks remain: - Making this same switch in `build:watch` - Parallelizing the builds, both locally and in CI - Perhaps applying the new process to our CDN bundle builds - Generalized cleanup These will all be included in separate PRs, some in the immediate future and some in the hopefully-not-too-distant short-to-medium term.
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.
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'simportHelpersoption. This completes that change, by adding a rollup plugin to the build process which extracts the injected code and replaces it with an equivalentimportorrequirestatement. Because the import comes from@sentry/utils, the few packages which didn't already depend on it have had it added as a dependency.Note: Originally, the changes from #5051 were included in this PR, and that is reflected in a number of the comments below. For ease of reviewing, they were split into their own PR.
Ref: https://getsentry.atlassian.net/browse/WEB-858