Standardize the UI toolchain on npm and fix dependency classification - #50
amc-corey-cox wants to merge 5 commits into
Conversation
vite, @vitejs/plugin-react, recharts, and @nivo/sankey were declared as runtime dependencies, so anything consuming @tis-lab/study-palette-ui installed the whole Vite toolchain. None are needed at runtime — the site build inlines recharts and nivo, and vite is build-only. Also ignore lib/, which the library build writes.
The UI has been developed and published with npm since September, but CI, the Netlify build, and the dev script still ran bun, and the committed bun lockfiles had not been updated since March. Two resolvers over one package.json meant CI validated a dependency graph nobody ships. Netlify keeps its target, base, publish dir, and redirects — only the package manager in the build command changes.
✅ Deploy Preview for study-palette ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
🟡 Changes recommended
Dependencies imported by non-test UI source (recharts, @nivo/sankey) were moved to devDependencies, which can break production/consumer installs that omit dev deps unless the build/publish pipeline explicitly bundles them.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR standardizes the UI workflow on npm (removing bun from CI/Netlify/local dev) and adjusts dependency classification in ui/package.json/ui/package-lock.json to avoid shipping build tooling as runtime dependencies.
Changes:
- Move
vite/@vitejs/plugin-react(and some UI libs) out ofdependenciesand intodevDependencies, updating the lockfile accordingly. - Remove
ui/bun.lockand switch CI (test-frontend), Netlify, and./devto use Node 22 + npm (npm ci,npm run ...). - Ignore
lib/in.gitignore.
File summaries
| File | Description |
|---|---|
| ui/package.json | Reclassifies runtime vs dev dependencies for the UI package. |
| ui/package-lock.json | Regenerates lockfile to reflect dependency section changes. |
| ui/bun.lock | Removes bun lockfile to eliminate dual package-manager drift. |
| netlify.toml | Switches Netlify build command from bun to npm. |
| dev | Updates local dev bootstrap/run commands to npm. |
| .gitignore | Adds lib/ to ignored build artifacts. |
| .github/workflows/test-frontend.yaml | Switches CI setup from bun to Node 22 + npm, with npm caching. |
Review details
Files not reviewed (1)
- ui/package-lock.json: Generated file
- Files reviewed: 4/8 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@yn-huang @varun-divya — Yinuo, thanks for pulling Flagging a merge conflict it creates with #44, because it's the kind that's easy to resolve wrong. Once #44 is in It's a union, not a pick:
Happy to take that merge myself if it's easier. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The package currently uses React 19 in devDependencies while peerDependencies only allow React 18, which can cause peer install failures/warnings for React 19 consumers.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (1)
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes are cohesive and low-risk (dependency classification + toolchain standardization) and the updated workflow already scopes npm commands to ./ui.
Review effort: Lite
Findings: None
Resolved since last review (1)
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes consistently migrate tooling from bun to npm across CI/Netlify/dev and the dependency reclassification aligns with the package’s described build/publish model without introducing inconsistencies in the reviewed configuration.
Review effort: Lite
Findings: None
yn-huang
left a comment
yn-huang
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for the cleanup Corey!
Two pieces of scaffold cruft that are getting in the way of the packaging work in #44 and tis-lab/bdc-dp-core#4. Both date to the original March scaffold and are mine.
Build tooling was declared as runtime dependencies.
vite,@vitejs/plugin-react,recharts, and@nivo/sankeysat independencies, so every consumer of the published@tis-lab/study-palette-uiinstalled the whole Vite toolchain. In bdc-dp-core that was 127 extra packages — vite, esbuild, and every@esbuild/*and@rollup/rollup-*platform binary. None are needed at runtime: the library build inlines recharts and nivo intolib/index.js, and vite is build-only.react/react-domare deliberately left alone, sinceclean-uiis already moving those.The repo had two package managers. The UI has been developed and published with npm since September, but CI, the Netlify build, and
./devstill ran bun, andui/bun.lockhadn't been touched since March — it still named the package unscoped with only react and react-dom in it.bun installruns unfrozen, so CI stayed green while validating a dependency graph nobody ships. That divergence is what hid abuild:libfailure under bun (ajv@6hoisted overajv-draft-04'sajv@8).Netlify keeps its target, base, publish directory, redirects, and environment — only the package manager in the build command changes, so this doesn't touch the Cloudapps/OpenShift direction.
Verified locally on Node 22:
npm ci→build→test(7 passed) →lint, all clean from a coldnode_modules. The Netlify preview on this PR exercises the new build command directly.