[DEV] Enable Perseus within Local Dev Server#3821
Conversation
…Perseus within the local dev server.
|
Size Change: 0 B Total Size: 518 kB ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (bea0005) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3821If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3821If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3821 |
jeremywiebe
left a comment
jeremywiebe
left a comment
There was a problem hiding this comment.
I've partially reviewed this, but the more I did, the more I'm concerned about the amount of info about our Frontend repo that's being revealed here.
I actually think it might be a script that should live in that repo instead of here (especially as the really nitpicky patching stuff is relevant to that repo and less-so in Perseus).
Happy to talk when we review this together.
There was a problem hiding this comment.
I'm 99% sure you don't need to name this .mts because we're using swc-node. All other "TypeScript scripts" in this folder just use .ts extensions.
There was a problem hiding this comment.
I tried that, and it failed. I'm open to investigating this when we pair.
| process.exit(0); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
Stylistically, I find it much clearer (and easier to read/maintain) if all of the code that comprises the main part of hte script is in a main() function which is called from the bottom of the file.
As this file stands, it's very hard to discover where the behaviour starts and function definitions end.
Basically, there should be no code other than the last main() function call that executes as the file is parsed.
| // Main flow | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| const perseusGitResult = spawnSync( |
There was a problem hiding this comment.
Can we put this in a function?
| const packagesDir = path.join(PERSEUS_ROOT, "packages"); | ||
| for (const entry of fs.readdirSync(packagesDir)) { | ||
| const pkgDir = path.join(packagesDir, entry); | ||
| const pkgJsonPath = path.join(pkgDir, "package.json"); | ||
| if (!fs.existsSync(pkgJsonPath)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
You could use fast-glob here to make this a bit more efficient:
for (const pkgJson of await fg(PERSEUS_ROOT, "packages", "*", "package.json")) {| // Step 3: Write docker-compose.perseus.env (supplies PERSEUS_ROOT to docker compose) | ||
| // Step 4: Patch containers.ts and rspack.config.ts | ||
| // Step 5: Apply pnpm.overrides to package.json | ||
| console.log("Patching frontend config..."); | ||
| writePerseusEnvFile(); | ||
| patchContainerTs(); | ||
| patchRspackConfig(pkgDirs); | ||
| applyOverrides(overrides); |
There was a problem hiding this comment.
Can we put the comments with the steps?
| // Step 3: Write docker-compose.perseus.env (supplies PERSEUS_ROOT to docker compose) | |
| // Step 4: Patch containers.ts and rspack.config.ts | |
| // Step 5: Apply pnpm.overrides to package.json | |
| console.log("Patching frontend config..."); | |
| writePerseusEnvFile(); | |
| patchContainerTs(); | |
| patchRspackConfig(pkgDirs); | |
| applyOverrides(overrides); | |
| console.log("Patching frontend config..."); | |
| // Step 3: Write docker-compose.perseus.env (supplies PERSEUS_ROOT to docker compose) | |
| writePerseusEnvFile(); | |
| // Step 4: Patch containers.ts and rspack.config.ts | |
| patchContainerTs(); | |
| // Step 5: Apply pnpm.overrides to package.json | |
| patchRspackConfig(pkgDirs); | |
| applyOverrides(overrides); |
| // --------------------------------------------------------------------------- | ||
|
|
||
| const SENTINEL_START = | ||
| "// LOCAL PERSEUS DEV — inserted by utils/start-perseus-in-local-server.mts"; |
There was a problem hiding this comment.
I'd generate this filename/path to protect against refactors (guessing at this exact syntax):
| "// LOCAL PERSEUS DEV — inserted by utils/start-perseus-in-local-server.mts"; | |
| `/ LOCAL PERSEUS DEV — inserted by ${path.relative(PERSEUS_ROOT, __dirname)}/${__filename}`; |
| // Catalog resolution | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| function parseCatalogs( |
There was a problem hiding this comment.
I think we'd be better off using a yaml parsing library. We already do very similar parsing in the sync-dependencies.ts script in getCatalogMap().
perseus/utils/sync-dependencies.ts
Line 34 in aafb3b3
| for (const entry of fs.readdirSync(pkgDir)) { | ||
| if (entry === "package.json") { | ||
| continue; | ||
| } | ||
| fs.symlinkSync( | ||
| path.join(pkgDir, entry), | ||
| path.join(tmpPkgDir, entry), | ||
| ); | ||
| } |
There was a problem hiding this comment.
So we create a directory for each package in a tmp dir and symlink everything in that package's dir to the tmp package dir except the package.json?
| // Build shallow Perseus package copies in tmp dir with catalogs resolved | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| function buildPerseusTmpPackages( |
There was a problem hiding this comment.
nit: These would make great JSDoc comments... :)
| function buildPerseusTmpPackages( | |
| /** | |
| * Build shallow Perseus package copies in tmp dir with catalogs resolved | |
| */ | |
| function buildPerseusTmpPackages( |
| const m = String(spec).match(/^catalog:(.+)$/); | ||
| if (!m) { | ||
| continue; | ||
| } | ||
| const resolved = catalogs[m[1]]?.[name]; | ||
| if (!resolved) { | ||
| throw new Error( | ||
| `Could not resolve catalog:${m[1]} for ${name} in ${pkgDir}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
suggestion: Named capture groups make this type of thing a ton clearer.
| const m = String(spec).match(/^catalog:(.+)$/); | |
| if (!m) { | |
| continue; | |
| } | |
| const resolved = catalogs[m[1]]?.[name]; | |
| if (!resolved) { | |
| throw new Error( | |
| `Could not resolve catalog:${m[1]} for ${name} in ${pkgDir}`, | |
| ); | |
| } | |
| const m = String(spec).match(/^catalog:(?<catalog>.+)$/); | |
| if (!m) { | |
| continue; | |
| } | |
| const resolved = catalogs[m.groups("catalog")]?.[name]; | |
| if (!resolved) { | |
| throw new Error( | |
| `Could not resolve catalog:${m.groups("catalog")} for ${name} in ${pkgDir}`, | |
| ); | |
| } |
2 participants
Summary:
This PR adds a script that dynamically sets up the local development environment to work with local Perseus code. The script adds the Perseus build directory to the Docker container so that when anything in Perseus changes, the package is rebuilt and HMR (hot module reload) picks up the changes and refreshes the browser page. The script does a lot of "configuring" that is required to handle things that sym-linking won't accomplish. For instance, the catalog entries for the various libraries used in Perseus are hard-wired so that Docker can pick up those libraries accordingly. The script needs to update a couple of files in the Frontend repo so that it knows how to properly reference the Perseus items. When the
stopcommand is executed, those changes are reverted.Test plan:
pnpm start:frontendlaunches the local dev server, and changes in Perseus code are reflected in the browser window showing the dev server.pnpm stop:frontendstops the local dev server, and reverts all the changes made to the Frontend repo needed to accommodate the Perseus references.