Hoist import declarations when transforming to commonjs/amd/umd module#39764
Closed
uhyo wants to merge 3 commits into
Closed
Hoist import declarations when transforming to commonjs/amd/umd module#39764uhyo wants to merge 3 commits into
uhyo wants to merge 3 commits into
Conversation
Merged
4 tasks
dbjorge
added a commit
to microsoft/accessibility-insights-web
that referenced
this pull request
Jun 4, 2021
#### Details Profiling our unit tests showed that the overwhelming majority of time spent was in `ts-jest` parsing typescript files. This PR is an experiment in replacing `ts-jest` with an alternative typescript transpiler, `swc`, which is less mature but much faster. ##### Motivation On my local machine (a Surface Laptop 3 running Windows 21H1 OS Build 19043.985), comparative timings are: | scenario | `main` (ts-jest) | this PR (swc) | | - | - | - | | `yarn test -- -- --coverage=false -- footer-section.test.tsx` | 12s ±1s | 4.3s ±0.2s | | `yarn test -- -- -- footer-section.test.tsx` | 45s ±3s | 6.2s ±0.0s | | `yarn test` | 11m54s | 3m ±14s | On our CI build agents: | scenario | `main` (ts-jest) | this PR (swc) | | - | - | - | | unit tests | [15m1s](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build/results?buildId=21734&view=logs&j=3613c2bd-8b62-5a70-8491-3e8459586450&t=d1343856-9cd9-5649-cdc6-81bc16053e6f) | [5m9s](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build/results?buildId=21738&view=logs&j=3613c2bd-8b62-5a70-8491-3e8459586450&t=d1343856-9cd9-5649-cdc6-81bc16053e6f) | | web-e2e tests | [10m10s](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build/results?buildId=21734&view=logs&j=952ed597-794a-5756-0328-a93bb2daa2a4&t=46864a42-71df-5c2d-c60f-5fec8d56499d) | [8m6s](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build/results?buildId=21738&view=logs&j=952ed597-794a-5756-0328-a93bb2daa2a4&t=46864a42-71df-5c2d-c60f-5fec8d56499d) | | unified-e2e tests | [10m39s](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build/results?buildId=21734&view=logs&j=c5567cdb-c930-534d-6489-7c2dcf7ac8ee&t=fe6db478-348c-54d0-bafe-001288e28283) | [10m33s](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build/results?buildId=21738&view=logs&j=c5567cdb-c930-534d-6489-7c2dcf7ac8ee&t=fe6db478-348c-54d0-bafe-001288e28283) | | total PR build time | [~22m](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build?definitionId=38&_a=summary) | [~14m](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build/results?buildId=21740&view=results) | ##### Context This is not as clear cut a transition as the timings above suggest. `swc` is a relatively young project; it's not quite as firmly single-maintainer as `esbuild`, but it's still got a fairly concerning bus factor compared to `tsc` or `babel` (what `ts-jest` hands off to). Still, the timings speak for themselves and it's easy enough to transition back if that ever becomes necessary. If this experiment goes well, we might consider a similar change to update our webpack config to use `swc-loader` instead of `ts-loader`. One of the reasons `swc` is so much faster is that it does not perform type-checking. Right now we do type-checking as part of build, but if we were to switch webpack to use swc, we'd then need a separate build step for type-checking (comparable to the current `yarn null:check` step). Implementation notes: * There are a few subtly important things in the `.swcrc` file, which unfortunately doesn't support comments: * The `noInterop: true` flag is the equivalent of TypeScript's default `esModuleInterop: false` behavior. It is necessary; we use several modules which cannot be used correctly with interop behavior in play. See swc-project/swc#1778 for details. * The empty `"env": {}` property is necessary to trigger `swc` to respect the `browserslist` entry in `package.json` * Directing `swc` to target a reasonably recent set of browsers (via the new `browserslist` entry in `package.json`) is required; there are several tests with snapshots that change under default targeting, and there are a few components that break outright. * `tsx: true` is required (it tells `swc` to enable support for `tsx` files; it doesn't prevent non-tsx files from parsing) * For now, this is using a custom Jest transformer to call `@swc/core`. The swc project maintains a `@swc/jest` transformer, but swc-project/jest#8 prevents it from being able to use the `noInterop: true` setting we require, and I can't submit a PR to patch the project until legal rubber stamps their CLA * `collapsible-script-provider.tsx` required a hacky workaround for swc-project/swc#1165 * even after applying the workaround, running jest with and without `--coverage=false` emitted javascript with different whitespace. `collapsible-script-provider.test.ts` needed to be updated to format the resulting js before snapshotting it. * `src/scanner/processor.ts` used a mutable global in a module to enable its tests to inject test data. This confused swc; I replaced it with a more typical ctor injection pattern. * `browser-adapter-factory.test.tsx` did a dance around an `import 'webextension-polyfill-ts';` statement to trick the module into thinking `window.chrome.runtime.id` is defined (it checks for this during import and refuses to load if it is not set). The dance we used is not compliant with how ES6 imports are defined to work (they are defined as hoisting import statements to the top of the file, ie, before we stub the window property). TypeScript has an outstanding PR/issue to stop supporting what we were previously doing (microsoft/TypeScript#39764) and swc intentionally doesn't support it (swc-project/swc#1686) (babel doesn't either), so I had to modify how our dance worked by moving it to a separate file and having it use a raw `require()` rather than an `import`. #### Pull request checklist <!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox --> - [n/a] Addresses an existing issue: #0000 - [x] Ran `yarn fastpass` - [x] Added/updated relevant unit test(s) (and ran `yarn test`) - [x] Verified code coverage for the changes made. Check coverage report at: `<rootDir>/test-results/unit/coverage` - [x] PR title *AND* final merge commit title both start with a semantic tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`). See `CONTRIBUTING.md`. - [n/a] (UI changes only) Added screenshots/GIFs to description above - [n/a] (UI changes only) Verified usability with NVDA/JAWS
Member
|
Unfortunately, we never finished reviewing this PR. It is pretty old now, so I'm going to close it to reduce the number of open PRs. |
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Fixes #16166.
So I was writing a TypeScript book and I wanted to demonstrate how the position of an
importdeclaration does not matter, but then encountered #16166.Here is a fix; this PR hoists all
importdeclarations when transforming commonjs/amd/umd modules.Although this is a breaking change, I see no concerns from TypeScript Team on that, looking at #16166 and #22178.
Example
Current Behavior
New Behavior