fix(tests): Add const-to-var ts-jest transformer
#5022
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
size-limit report 📦
|
lobsterkatie
force-pushed
the
kmclb-add-const-to-var-ts-jest-transformer
branch
from
0be9af5 to
f55e0e9
Compare
AbhiPrasad
approved these changes
Apr 30, 2022
lobsterkatie
force-pushed
the
kmclb-add-const-to-var-ts-jest-transformer
branch
from
f55e0e9 to
650f2ca
Compare
This was referenced May 2, 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
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 does, during tests, what the
const-to-varplugin in #4993 does for our built code. Though for tests bundle size isn't a concern, the first point raised there (of our shadowing ofglobalbeing an issue now that we're ES6ified and living in a land ofconsts) stands, though in this case it seems to only affect Node 8.Since
ts-jestdoesn't use our built code, but instead compiles our TS on the fly, the existing plugin is of no help. Hence this transformer, whichts-jestwill apply after it has transpiled our code to JS. It works by providing a visitor function whichts-jestpasses on to TS, which then lets it loose on the AST. In our case (as you'd expect) our visitor stops on everyconstvariable declaration node and replaces it with an equivalentvardeclaration 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.