Improve build performance #733
Conversation
hmorgancode
force-pushed
the
183111062_build_perf_for_real_this_time
branch
from
5893c2b
to
1a21c31
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #733 +/- ##
===========================================
+ Coverage 55.33% 81.52% +26.18%
===========================================
Files 203 208 +5
Lines 8421 8448 +27
Branches 2290 2290
===========================================
+ Hits 4660 6887 +2227
+ Misses 3595 1486 -2109
+ Partials 166 75 -91
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 108 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
hmorgancode
force-pushed
the
183111062_build_perf_for_real_this_time
branch
from
b362bb3
to
9867986
Compare
There was a problem hiding this comment.
While I'm pleased by the performance gains here, I have some suggestions moving forward. Given that the PR has already been merged, these suggestions will have to be in a subsequent PR.
These configurations are subtle and the circumstances around them change. CC used to regularly use ts-loader (in transpileOnly mode) in combination with the fork-ts-checker-webpack-plugin until it became apparent that the relatively small performance improvement didn't warrant the additional configuration complexity. It's important to document the rationale for the decisions being made so that they can be revisited when circumstances change. As such, I've flagged a number of places where comments should be added explaining the potential costs/benefits and the rationale for choosing one approach over another. Citing references with recommendations would be helpful as well.
Furthermore, I'd like to understand the relative value of the various changes being made. How much of the improvement is due to caching changes vs. the change from ts-loader to swc-loader? Is this the preferred cache configuration or might there be better options? (See more detailed question inline.) Ultimately, each individual change can/should be evaluated based on its merits and sufficient information provided to indicate when it should be reevaluated. That said, thanks for looking into this and I look forward to taking advantage of the performance improvements.
| - uses: actions/cache@v3 | ||
| with: | ||
| path: v3/node_modules/.cache/webpack/ | ||
| key: ${{ github.head_ref || github.ref_name }}-webpack-build | ||
| restore-keys: | | ||
| main-webpack-build |
There was a problem hiding this comment.
The documentation suggests that setup-node can automatically cache what needs to be cached:
Alternatively, if you are caching the package managers listed below, using their respective
setup-*actions requires minimal configuration and will create and restore dependency caches for you.
The documentation for actions/setup-node suggests that simply adding cache: true will have similar effect, potentially caching the entire node_modules folder. So it seems like caching is a good idea, but I think it would be good to understand and measure the difference between just specifying cache: true to setup-node and separately configuring actions/cache. And then the final choice should be documented with local comments so that if things change down the road the situation can be revisited.
| @@ -87,7 +87,7 @@ export const useDropHandler = ({ selector, onImportDataSet, onImportV2Document, | |||
|
|
|||
| function removeDragData(event: DragEvent) { | |||
| if (event.dataTransfer) { | |||
| if (event.dataTransfer.items) { | |||
| if (event.dataTransfer?.items?.clear) { | |||
There was a problem hiding this comment.
if (event.dataTransfer.items?.clear) {You don't need the optional chain here since event.dataTransfer was tested on the previous line.
| loader: 'ts-loader', | ||
| test: /.(ts|tsx)$/, | ||
| include: path.resolve(__dirname, "src"), | ||
| use: process.env.CODE_COVERAGE ? { loader: "ts-loader" } : { |
There was a problem hiding this comment.
Add comment explaining use of ts-loader for code coverage.
| parser: { | ||
| syntax: "typescript", | ||
| decorators: true, | ||
| tsx: false, |
There was a problem hiding this comment.
Add comment as this setting is counter-intuitive.
| optimization: devMode ? { | ||
| removeAvailableModules: false, | ||
| removeEmptyChunks: false, | ||
| splitChunks: false, | ||
| } : {}, |
There was a problem hiding this comment.
I would have thought that Webpack was smart enough to skip these optimizations in devMode automatically. Did you test this and/or is there documentation to suggest otherwise?
| if (!process.env.CODE_COVERAGE) { | ||
| webpackPlugins.push(new ForkTsCheckerWebpackPlugin()) | ||
| } |
There was a problem hiding this comment.
Comment here on the use of ForkTsCheckerWebpackPlugin would be useful.
This does several things:
-Use the much faster SWC compiler for our typescript files (except when building for code coverage on CI)
-Since swc doesn't do it, move type checking to a separate process. (the alert when a type error occurs is explanatory and visible)
-Limit SWC to just checking files in the 'src' directory (this saved more time than I thought it would)
-Minor optimizations when in dev mode
-Cache webpack builds on the filesystem, so even if you exit webpack-dev-server you should have a faster build.
-Cache webpack builds from Github Actions on the same branch (defaulting to main if a cache does not exist)
NOTE: If you don't exit webpack-dev-server gracefully (i.e. if you spam CTRL+C), the cache will be invalidated and the next build will be a cold one. CTRL+C and then hitting Y when prompted should be safe.