fix(build): Ensure bundle builds have needed dependencies #5119
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 📦
|
AbhiPrasad
approved these changes
May 17, 2022
scripts/ensure-bundle-deps.ts
Outdated
| // Checking that the directory exists isn't 100% the same as checking that the files themselves exist, of course, | ||
| // but it's a decent proxy, and much simpler to do than checking for individual files. (We're arbitrarily checking | ||
| // for the `cjs` directory here rather than the `esm` directory; they're built together so either one will do.) | ||
| if (!fs.existsSync(`${depBuildDir}/cjs`) && !fs.existsSync(`${depBuildDir}/npm/cjs`)) { |
There was a problem hiding this comment.
we could just as easily check for both esm and cjs - might be worthwhile just to have anyway.
There was a problem hiding this comment.
Actually, you're reminding me that it's actually the esm that it uses, because we use the default for this @rollup/plugin-node-resolve option. Added it to the check.
scripts/ensure-bundle-deps.ts
Outdated
| }): Promise<void> { | ||
| const { maxRetries = 12, dependencies } = options; | ||
|
|
||
| const processDotEnv = process.env as ProcessDotEnv; |
There was a problem hiding this comment.
I mean I would rather just treat these as possible to be undefined and do existence checks on INIT_CWD and npm_config_argv instead of using a custom ProcessDotEnv type. Let's just play on the safe side here.
const initCwd = process.env.INIT_CWD;
if (initCwd === undefined) {
console.error("INIT_CWD environment variable not defined");
return;
}
const npmConfigArgv = process.env.npm_config_argv;
if (npmConfigArgv === undefined) {
console.error("npm_config_argv environment variable not defined");
return;
}There was a problem hiding this comment.
IDK, I feel like the only way this breaks is if we upgrade yarn (at which point we'd know to be watching for breakage), and the worst that happens if it does is our build won't go. Feels fairly low stakes to me. But, anyway, added a check.
|
|
||
| console.log('\nSearching for bundle dependencies...'); | ||
|
|
||
| while (retries < maxRetries && !checkForBundleDeps(packagesDir, dependencyDirs)) { |
There was a problem hiding this comment.
nit: This should really be a helper func which just retries a generic func a certain num of times.
There was a problem hiding this comment.
Meh? I feel like there's enough bespoke stuff here that refactoring it into a generic retry-er would take some effort, and AFAIK we don't have a general need for such a thing at the moment. I'd rather put off that work until it's actually needed. (Which, to be fair, will happen if we try to apply this strategy to other parts of the build. But let's handle that then, so we can get this fix in.)
| /** | ||
| * Wait the given number of milliseconds before continuing. | ||
| */ | ||
| async function sleep(ms: number): Promise<void> { |
There was a problem hiding this comment.
looking forward to us having this dev-utils package!
There was a problem hiding this comment.
Same! That run function is in practically every script I write, LOL.
lobsterkatie
force-pushed
the
kmclb-fix-repo-level-bundle-build-race-condition
branch
3 times, most recently
from
a464a86 to
74c45ff
Compare
lobsterkatie
force-pushed
the
kmclb-fix-repo-level-bundle-build-race-condition
branch
from
74c45ff to
22bdad2
Compare
AbhiPrasad
pushed a commit
that referenced
this pull request
May 30, 2022
In #5094, a change was made to parallelize our repo-level build commands as much as possible. In that PR, it was stated that `build:bundle` could run independent of, and therefore in parallel with, the types and rollup builds, and at the time that was true. When TS (through rollup) builds a bundle, it creates a giant AST out of the bundled package and all of its monorepo dependencies, based on the original source code, then transpiles the whole thing - no prework needed. But in #5111 we switched our ES6 bundles to use sucrase for transpilation (still through rollup), and that changed things. Sucrase (along with every other non-TS transpilation tool) only considers files one by one, not as part of a larger whole, and won't reach across package boundaries, even within the monorepo. As a result, rollup needs all dependencies to already exist in transpiled form, since sucrase doesn't touch them, which becomes a problem if both processes are happening at once. (_But how has CI even been passing since that second PR, then?_, you may ask. The answer is, sucrase is very fast, and lerna can only start so many things at once. It ends up being a race condition between sucrase finishing with the dependencies and lerna kicking off the bundle builds, and almost all the time, sucrase wins. And the other situations in which this is broken all involve using something other than the top-level `build` script to create bundles in a repo with no existing build artifacts, and CI just never does that.) So TL;DR, we need to have already transpiled a packages's monorepo dependencies before that package can be turned into a bundle. For `build:bundle` at both the repo and package level, and for `build` at the package level, this means that if they're not there, we have to build them. For `build` at the repo level, where transpilation of all packages does in fact already happen, we have two options: 1) Push bundle builds to happen alongside `build:extras`, after rollup builds have finished. 2) Mimic what happens when the race condition is successful, but in a way that isn't flaky. In other words, continue to run `build:bundle` in parallel with the types and npm package builds, but guarantee that the needed dependencies have finished building themselves before starting the bundle build. Of the two options, the first is certainly simpler, but it also forces the two longest parts of the build (bundle and types builds) to be sequential, which is the exact _opposite_ of what we want, given that the goal all along has been to make the builds noticeably faster. Choosing the second solution also gives us an excuse to add the extra few lines of code needed to fix the repo-level-build:bundle/package-level-build/package-level-build:bundle problem, which we otherwise probably wouldn't fix. This implements that second strategy, by polling at 5 second intervals for the existence of the needed files, for up to 60 seconds, before beginning the bundle builds. (In practice, it fairly reliably seems to take two retries, or ten seconds, before the bundle build can begin.) It also handles the other three situations above, by building the missing files when necessary. Finally, it fixes a type import to rely on the main types package, rather than a built version of it. This prevents our ES5 bundles (which still use TS for transpilation, in order to also take advantage of its ability to down-compile) from running to the same problem as the sucrase bundles are having.
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.
In #5094, a change was made to parallelize our repo-level build commands as much as possible. In that PR, it was stated that
build:bundlecould run independent of, and therefore in parallel with, the types and rollup builds, and at the time that was true. When TS (through rollup) builds a bundle, it creates a giant AST out of the bundled package and all of its monorepo dependencies, based on the original source code, then transpiles the whole thing - no prework needed.But in #5111 we switched our ES6 bundles to use sucrase for transpilation (still through rollup), and that changed things. Sucrase (along with every other non-TS transpilation tool) only considers files one by one, not as part of a larger whole, and won't reach across package boundaries, even within the monorepo. As a result, rollup needs all dependencies to already exist in transpiled form, since sucrase doesn't touch them, which becomes a problem if both processes are happening at once.
(But how has CI even been passing since that second PR, then?, you may ask. The answer is, sucrase is very fast, and lerna can only start so many things at once. It ends up being a race condition between sucrase finishing with the dependencies and lerna kicking off the bundle builds, and almost all the time, sucrase wins. And the other situations in which this is broken all involve using something other than the top-level
buildscript to create bundles in a repo with no existing build artifacts, and CI just never does that.)So TL;DR, we need to have already transpiled a packages's monorepo dependencies before that package can be turned into a bundle. For
build:bundleat both the repo and package level, and forbuildat the package level, this means that if they're not there, we have to build them. Forbuildat the repo level, where transpilation of all packages does in fact already happen, we have two options:build:extras, after rollup builds have finished.build:bundlein parallel with the types and npm package builds, but guarantee that the needed dependencies have finished building themselves before starting the bundle build.Of the two options, the first is certainly simpler, but it also forces the two longest parts of the build (bundle and types builds) to be sequential, which is the exact opposite of what we want, given that the goal all along has been to make the builds noticeably faster. Choosing the second solution also gives us an excuse to add the extra few lines of code needed to fix the repo-level-build:bundle/package-level-build/package-level-build:bundle problem, which we otherwise probably wouldn't fix.
This implements that second strategy, by polling at 5 second intervals for the existence of the needed files, for up to 60 seconds, before beginning the bundle builds. (In practice, it fairly reliably seems to take two retries, or ten seconds, before the bundle build can begin.) It also handles the other three situations above, by building the missing files when necessary. Finally, it fixes a type import to rely on the main types package, rather than a built version of it. This prevents our ES5 bundles (which still use TS for transpilation, in order to also take advantage of its ability to down-compile) from running to the same problem as the sucrase bundles are having.