feat(build): Add various rollup plugins to sucrase build #4993
Conversation
This file contains 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
size-limit report 📦
|
lobsterkatie
force-pushed
the
kmclb-add-various-rollup-plugins-sucrase-build
branch
3 times, most recently
from
30ac75d
to
5a0d4ab
Compare
11 tasks
lobsterkatie
force-pushed
the
kmclb-add-various-rollup-plugins-sucrase-build
branch
7 times, most recently
from
e0fbd7d
to
31d68b8
Compare
AbhiPrasad
approved these changes
Apr 28, 2022
| * | ||
| * @returns A `rollup-plugin-re` instance. | ||
| */ | ||
| export function makeRemoveESLintCommentsPlugin() { |
There was a problem hiding this comment.
Can we add a make a ticket somewhere to add tests for these plugins?
There was a problem hiding this comment.
Done, though of all of our build steps, I think there are many which more urgently need testing.
lobsterkatie
force-pushed
the
kmclb-add-various-rollup-plugins-sucrase-build
branch
from
31d68b8
to
a25873c
Compare
lobsterkatie
added a commit
that referenced
this pull request
May 2, 2022
This does, during tests, what the `const`-to-`var` plugin in #4993 does for our built code. Though for tests bundle size isn't a concern, the first point raised there (of our shadowing of `global` being an issue now that we're ES6ified and living in a land of `const`s) stands, though in this case it seems to only affect Node 8. Since `ts-jest` doesn't use our built code, but instead compiles our TS on the fly, the existing plugin is of no help. Hence this transformer, which `ts-jest `will apply after it has transpiled our code to JS. It works by providing a visitor function which `ts-jest` passes on to TS, which then lets it loose on the AST. In our case (as you'd expect) our visitor stops on every `const` variable declaration node and replaces it with an equivalent `var` declaration node, and ignores all other nodes. Since it's only needed for node 8, we leave transpilation of the transformer itself and injection into our jest config until runtime on the Node 8 CI machine. As part of this work, the script which runs tests in CI (whose main job is to ensure compatibility with older versions of Node, and to which the aforementioned transpilation and injection were added) has been reorganized into functions, for (hopefully) better readability.
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
This adds four rollup plugins for use in the new build process:
- A plugin which converts all `const`s in the output to `var`s, both after sucrase's code modifications and after those done by rollup itself, to make sure that all instances of `const` are caught. This transformation has two advantages:
- It prevents errors arising from the way we redefine `global` (as the return value of `getGlobalObject`), since `var`s can be redeclared where `const`s can't.
- It replaces a very common token which is four letters with one which is three letters. Not a huge bundle size savings, but it at least prevents us from going the wrong direction. (Our current builds use `var` because they are ES5, so this puts the new build in line with the current build, number-of-characters-in-variable-specifiers-wise.)
- A plugin which strips eslint-style comments and another which reduces multiple consecutive blank lines down to one. Neither of these affects either code behavior or bundle size, but they do make the eventual output a little less messy. One of the odd quirks of sucrase is that it has the goal of never changing the line number of an expression[1], in order that even stacktraces from un-sourcemapped code are meaningful, and this can result in some odd-looking files. These two plugins don't totally undo the oddness, but they're simple and fast and certainly don't hurt readability.
- A plugin to be used during our development, which allows you to place a debugger in the rollup hook[2] of your choice.
[1] alangpierce/sucrase#452 (comment)
[2] https://rollupjs.org/guide/en/#build-hooks
AbhiPrasad
pushed a commit
that referenced
this pull request
May 30, 2022
This does, during tests, what the `const`-to-`var` plugin in #4993 does for our built code. Though for tests bundle size isn't a concern, the first point raised there (of our shadowing of `global` being an issue now that we're ES6ified and living in a land of `const`s) stands, though in this case it seems to only affect Node 8. Since `ts-jest` doesn't use our built code, but instead compiles our TS on the fly, the existing plugin is of no help. Hence this transformer, which `ts-jest `will apply after it has transpiled our code to JS. It works by providing a visitor function which `ts-jest` passes on to TS, which then lets it loose on the AST. In our case (as you'd expect) our visitor stops on every `const` variable declaration node and replaces it with an equivalent `var` declaration node, and ignores all other nodes. Since it's only needed for node 8, we leave transpilation of the transformer itself and injection into our jest config until runtime on the Node 8 CI machine. As part of this work, the script which runs tests in CI (whose main job is to ensure compatibility with older versions of Node, and to which the aforementioned transpilation and injection were added) has been reorganized into functions, for (hopefully) better readability.
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 adds four rollup plugins for use in the new build process:
A plugin which converts all
consts in the output tovars, both after sucrase's code modifications and after those done by rollup itself, to make sure that all instances ofconstare caught. This transformation has two advantages:global(as the return value ofgetGlobalObject), sincevars can be redeclared whereconsts can't.varbecause they are ES5, so this puts the new build in line with the current build, number-of-characters-in-variable-specifiers-wise.)A plugin which strips eslint-style comments and another which reduces multiple consecutive blank lines down to one. Neither of these affects either code behavior or bundle size, but they do make the eventual output a little less messy. One of the odd quirks of sucrase is that it has the goal of never changing the line number of an expression, in order that even stacktrces from un-sourcemapped code are meaningful, and this can result in some odd-looking files. These two plugins don't totally undo the oddness, but they're simple and fast and certainly don't hurt readability.
A plugin to be used during our development, which allows you to place a debugger in the rollup hook of your choice.