Skip to content

chore(ailab): migrate to vite + vitest + typescript toolchain#73010

Open
sanchitmalhotra126 wants to merge 1 commit into
ailab-migrationfrom
ailab-vite-toolchain
Open

chore(ailab): migrate to vite + vitest + typescript toolchain#73010
sanchitmalhotra126 wants to merge 1 commit into
ailab-migrationfrom
ailab-vite-toolchain

Conversation

@sanchitmalhotra126
Copy link
Copy Markdown
Contributor

@sanchitmalhotra126 sanchitmalhotra126 commented Jun 2, 2026

Why

Follow-up #1 to the mechanical migration (#72976), which inlined @code-dot-org/ml-playground into frontend/packages/labs/ailab/ but deliberately left its standalone webpack + babel + jest toolchain in place (and therefore out of the turbo build/typecheck pipeline).

This PR swaps that toolchain for the monorepo-standard vite + vitest + tsc setup (mirroring frontend/packages/labs/oceans), restoring a green build and typecheck and wiring the package into turbo so CI sees it.

Stacked on ailab-migration. Base this against that branch; it retargets to staging once #72976 merges.

What this PR does

  • Build — a vite library build emits dist/index.{mjs,cjs,d.ts} (replacing the old webpack multi-entry output). package.json gains exports/main/module/types pointing at dist/, plus build/dev/typecheck scripts so the package joins the turbo pipeline.
  • Externalizationreact, react-dom, redux, react-redux move to peerDependencies and are externalized (vite-plugin-externalize-deps), so the consumer's single instances are used at runtime — preserving the redux "one instance per page" invariant the old webpack externals documented. Everything else (chart.js, ml-knn, papaparse, messageformat, …) is bundled.
  • Assets — the five UI images are inlined as data-URIs (library mode), so they carry no runtime path. The 132 dataset files are emitted to dist/assets/datasets/ by a small rollup plugin for the host to serve; a dev-only middleware serves them at /datasets/ for the standalone harness. index.html moved to the package root (vite's dev entry); indexDev.tsx is the dev harness.
  • setAssetPath simplification — deleted setPublicPath.ts and the __webpack_public_path__ machinery. With images inlined, nothing needs a webpack public path; setAssetPath now governs only the runtime dataset-fetch base, and its call-ordering contract relaxes from "before importing the bundle" to "before initAll()."
  • Tests — jest → vitest, 108 passing. .test.js files get vitest globals via the shared config.
  • Lint/format — adopt the shared flat eslint.config.mjs + prettier config; deleted .eslintrc.js, babel.config.json, the local prettier index.mjs, webpack.config.js, src/indexProd.tsx.

Decisions worth a look

  • Strictness deferral. The migrated source predates a few shared-config checks. To keep this swap mechanical, they're relaxed in this package only, with comments pointing at the cleanup follow-up:
    • tsconfig.app.json: verbatimModuleSyntax, noUnusedLocals, noUnusedParameters off (~110 import type + ~16 unused-binding violations).
    • eslint.config.mjs: import-x/no-cycle (real store↔index↔component cycles), @typescript-eslint/no-unused-vars, @typescript-eslint/no-explicit-any, and two import-x CJS-default-interop false positives on react/papaparse. import-x/order stays on (auto-fixed across the source — most of the source diff is this reordering).
  • The one genuine type error the migration deferred was a real papaparse parse() overload mismatch (the source imports papaparse directly but only react-papaparse was declared). Fixed properly: declare papaparse + @types/papaparse, drop the dead react-papaparse shim, and split the call by the download flag so each branch matches a real overload.

What this PR does NOT do

  • No consumer rewiring. apps/ still imports the npm @code-dot-org/ml-playground and the Gruntfile still copies datasets from it. Nothing in the studio build consumes this package yet → near-zero risk. Repointing AilabView.tsx + the Gruntfile to @code-dot-org/ailab (and sequencing the frontend build before the apps build) is follow-up store sprite.value again, use it properly in setSpriteSize #2.
  • No e2e/Playwright suite — deferred to a later update (see follow-ups).

Test plan

  • CI: turbo run build typecheck test lint prettier --filter=@code-dot-org/ailab green (verified locally — 8/8 tasks).
  • yarn workspace @code-dot-org/ailab dev serves the standalone harness; datasets load and AI-bot images render (verified locally: /datasets/*.csv 200 via middleware, images inlined).
  • dist/ contains index.{mjs,cjs,d.ts} exporting initAll/instructionsDismissed/setAssetPath, plus assets/datasets/ (132 files).
  • Studio unaffected — apps/ still on npm @code-dot-org/ml-playground.

Follow-ups

  1. Consumer rewire (store sprite.value again, use it properly in setSpriteSize #2): point apps/ at the workspace package; ensure the frontend build runs before the apps Grunt copy.
  2. Strictness cleanup: restore the relaxed tsconfig flags + eslint rules and untangle the import cycles.
  3. Playwright e2e suite against the standalone dev harness, following oceans (refactor(oceans-lab): swap toolchain to vite + lint-config + tsx + Playwright (stacked on #72539) #72579): a smoke spec per sample mode (boots clean, datasets load, expected panel renders), then 1–2 train→predict happy paths, plus an axe a11y pass. Infra is already present (playwright + @axe-core/playwright in the catalog, a test:ui:ci turbo task). Tracked for a later update.
  4. Consider folding assetPath into initAll(...) options to retire the globalThis write entirely (kills the last ordering caveat).

🤖 Generated with Claude Code

Follow-up #1 to the mechanical migration (#72976). Replace the package's
webpack + babel + jest toolchain with the monorepo-standard vite + vitest +
tsc setup, restoring a green build/typecheck and wiring the package into the
turbo pipeline. No consumer rewiring: apps/ still pulls npm
@code-dot-org/ml-playground, so nothing in the studio build changes yet.

Build: a vite library build emits dist/index.{mjs,cjs,d.ts}. peerDependencies
(react, react-dom, redux, react-redux) are externalized so the consumer's
single instances are used; everything else is bundled. The five UI images are
inlined as data-URIs (no runtime path), and the 132 dataset files are emitted
to dist/assets/datasets/ for the host to serve. A dev-only middleware serves
them at /datasets/ for the standalone harness (index.html moved to the package
root as the vite entry). Deleted the webpack __webpack_public_path__ machinery
(setPublicPath.ts): with images inlined, setAssetPath now governs only the
runtime dataset URLs.

Tests: jest -> vitest, 108 passing. Lint/prettier adopt the shared flat
eslint + prettier config; the .test.js suite gets vitest globals.

The migrated source predates some shared-config strictness. Relaxed in this
package with a documented, tracked cleanup follow-up: tsconfig
verbatimModuleSyntax + noUnusedLocals/Parameters off; eslint import-x/no-cycle,
no-unused-vars, no-explicit-any, and two react/papaparse CJS-interop
false-positives off. The one genuine type error (a papaparse parse() overload)
is fixed properly.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sanchitmalhotra126 sanchitmalhotra126 requested review from a team, stephenliang and wilkie June 2, 2026 23:28
Copy link
Copy Markdown
Member

@stephenliang stephenliang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A friendly pass over the vite + vitest + typescript migration 👋

Thanks for taking this on — toolchain swaps are fiddly, thankless work, and the overall shape here is really solid. I went through it three times wearing different hats (Vite/bundling, Vitest, TypeScript) and wanted to share notes in the spirit of "here's what I'd polish with you," not a gate. I've left inline comments with concrete, applyable suggestions wherever I could.

Tests — nicely done. I actually ran the suite (vitest --run): 108 passing across 13 files, clean exit, no hangs. I also poked it by injecting a deliberately-failing assertion to confirm the tests genuinely assert (they do). The only changes to the *.test.js files are import reordering — you carried the behavior over faithfully, which is exactly what you want in a migration like this. Lovely and disciplined.

TypeScript — also in good shape. strict is still on, the typecheck is genuinely green, and the papaparse fix is correct: the original error was real (the non-literal download flag matched no overload, and it only compiled before because papaparse was an untyped phantom dep), and your branch split lines up with the real @types/papaparse overloads with no casts. I re-enabled the deferred tsconfig flags in a throwaway copy just to be sure nothing scary was hiding — it's all cosmetic (≈111 import type-style + 16 unused bindings), no real bugs. The deferral-plus-follow-up plan reads as very reasonable.

A few Vite details I'd love to look at together (all inline, with fixes):

  1. The "five UI images are small" comment isn't quite holding up in practice — about 626 KB of base64 is landing in the bundle. The oceans package has a nice pattern we can borrow.
  2. vite build is exiting 0 even though the dts step logs a couple of TS6307 errors — easy one-line fix, and I left the exact suggestion.
  3. The dev /datasets/ middleware can be walked out of its directory with ../ — dev-only, but a quick guard closes it.
  4. A tiny stale lodash mention in a comment (it rode along from oceans).

None of these are a knock on the work — they're exactly the kind of thing that's easy to miss in a big mechanical swap. Happy to pair on any of them!

🤖 Written by Claude Code.

Comment on lines +85 to +92
// Library mode inlines imported assets as data URIs (it cannot know the
// deploy base URL). The five UI images are small, so this is exactly what
// we want — they ride along in the JS with no runtime path dependency.
lib: {
entry: ['src/index.tsx'],
name: 'ailab',
formats: ['es', 'cjs'],
},
Copy link
Copy Markdown
Member

@stephenliang stephenliang Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I noticed when I built this locally: the comment says the five UI images are small, but a few are actually pretty hefty once inlined. dist/index.mjs comes out around 1,430 KB, and ~626 KB of that is base64 image data — blue-scanner.png (293 KB), results-background-light.jpg (117 KB), and ai-bot-border.png (47 KB). Library mode always inlines (it can't know the deploy URL, and it ignores assetsInlineLimit), so that base64 gets parsed on every page load and can't be cached separately. The old webpack setup kept these as separate cacheable files.

The nice part is that oceans already solves exactly this — it emitFiles its large binary instead of inlining. We could borrow that pattern here: import the big images with ?url and emit them through the same emitDatasets plugin you've already written, then resolve their path off setAssetPath like the datasets do. If that feels like more than you want in this PR, even just updating the comment so the next person isn't surprised would be a win. Totally workable either way — happy to pair on it.

🤖 Written by Claude Code.

emitDatasets(),
devDatasets(),
// Emit `.d.ts` from the library entry.
dts({tsconfigPath: './tsconfig.app.json', entryRoot: 'src'}),
Copy link
Copy Markdown
Member

@stephenliang stephenliang Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small one I bumped into when running the build: vite-plugin-dts logs a couple of TS6307 errors here — src/datasetManifest.ts:1 imports ../public/datasets-manifest.json and src/i18n.ts:3 imports ../i18n/mlPlayground.json, both living outside this tsconfig's include: ["src"]. The slightly sneaky part is that vite build still exits 0, so CI stays green even though the dts step is unhappy. It works today, but it's the kind of thing that could quietly break the published types after a future plugin bump. Good news — it's a one-liner; I left the exact fix as a suggestion over on tsconfig.app.json. 🙂

🤖 Written by Claude Code.

"@public/*": ["public/*"]
}
},
"include": ["src"]
Copy link
Copy Markdown
Member

@stephenliang stephenliang Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the fix for those TS6307 dts errors I mentioned on vite.config.ts:72 — just teaching the tsconfig about the two JSON files that get imported from outside src. Should be safe to apply directly:

Suggested change
"include": ["src"]
"include": ["src", "public/datasets-manifest.json", "i18n/mlPlayground.json"]

🤖 Written by Claude Code.

Comment on lines +43 to +46
const file = path.join(DATASETS_SRC, req.url.slice(prefix.length));
if (fs.existsSync(file)) {
req.url = `/@fs${file}`;
}
Copy link
Copy Markdown
Member

@stephenliang stephenliang Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One to tuck away for safety: because this does path.join(DATASETS_SRC, req.url…) on the raw URL, a request like GET /datasets/../../../../etc/passwd can normalize its way out of the datasets folder and then get served via /@fs. It's only the dev server on localhost, so the real-world risk is low — but it's a cheap one to close, and a nice habit to build in. Decoding, resolving, and checking the path stays under DATASETS_SRC does it:

Suggested change
const file = path.join(DATASETS_SRC, req.url.slice(prefix.length));
if (fs.existsSync(file)) {
req.url = `/@fs${file}`;
}
const rel = decodeURIComponent(req.url.slice(prefix.length));
const file = path.resolve(DATASETS_SRC, rel);
if (file.startsWith(DATASETS_SRC + path.sep) && fs.existsSync(file)) {
req.url = `/@fs${file}`;
}

🤖 Written by Claude Code.

Comment on lines +73 to +74
// Externalize peerDependencies only (react, react-dom, redux, react-redux,
// lodash) so the consumer's single instances are used at runtime; bundle
Copy link
Copy Markdown
Member

@stephenliang stephenliang Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny nit, no worries at all: this comment mentions lodash as an externalized peer dep, but it looks like it rode along from the oceans config — this package's peerDeps are just react/react-dom/redux/react-redux, and lodash isn't imported here. Trimming it keeps the comment honest for the next reader:

Suggested change
// Externalize peerDependencies only (react, react-dom, redux, react-redux,
// lodash) so the consumer's single instances are used at runtime; bundle
// Externalize peerDependencies only (react, react-dom, redux,
// react-redux) so the consumer's single instances are used at runtime; bundle

🤖 Written by Claude Code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants