fix(web): keep proto-ts installable after the workspace npm ci - #728
fix(web): keep proto-ts installable after the workspace npm ci#728MohammedAlkindi wants to merge 2 commits into
Conversation
bindings/proto-ts is a member of the bindings/web npm workspace but sits outside the workspace root, so 'npm ci' in bindings/web hoists its dependencies to bindings/web/node_modules — unreachable from bindings/proto-ts/src — and leaves bindings/proto-ts without its own node_modules. Every documented local flow (npm run build:ts, typecheck) then fails with TS2307 on @bufbuild/protobuf/wire. CI already repairs this with a standalone 'npm ci --workspaces=false' in proto-ts; run the same command as a postinstall on the private workspace root so the documented commands work on a clean clone.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe web package adds a root ChangesWeb package installation
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change makes the documented Web SDK install flow install the proto-ts dependencies needed for TypeScript builds; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description follows the repository template. It explains the problem, solution, testing results, scope, label, and checklist status. It also identifies non-applicable testing and documentation considerations. Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Description
On a clean clone, the documented Web SDK developer flow fails before any code is touched: after IDL codegen, running
npm ciinbindings/weband thennpm run build:ts(ortypecheck) dies with 39xTS2307: Cannot find module '@bufbuild/protobuf/wire'.bindings/proto-tsis a workspace member of thebindings/webnpm workspace but sits outside the workspace root, so the workspace install hoists proto-ts's dependencies intobindings/web/node_modules— a directory Node's resolution frombindings/proto-ts/srccan never reach — and leavesbindings/proto-tswithout its ownnode_modules.This is a known trap — CI already carries the workaround: the
web-typecheckjob inpr-build.ymlhas an explicit "Install proto-ts dependencies (standalone install)" step (npm ci --workspaces=falseinbindings/proto-ts) right after the workspace install, with a comment describing this exact mechanism. This PR ports that repair into the package so the documented local flow works too: apostinstallon the private@runanywhere/web-rootpackage runs the same command, sonpm ci/npm installinbindings/webleavesbindings/proto-tsable to compile itself. Today the repair exists only in workflow YAML — the rootAGENTS.md("Web SDK":cd bindings/web/,npm run build,npm run typecheck),CONTRIBUTING.md's./run sdk web build, andbindings/web/AGENTS.mdall describe the flow without it, so every fresh contributor checkout hits the TS2307 wall.Deliberately unchanged: the
web-typecheckworkflow's explicit repair step. It becomes redundant (the postinstall performs the same install) but is idempotent and harmless; dropping it can be a follow-up once this has baked.@runanywhere/web-rootis"private": trueand never published, so the script cannot reach consumers of the published packages.Type of Change
Testing
On Windows 11 (Node v24.18.0, npm 11.16.0 — CI's
web-typecheckuses v24.19.0 / npm 11.17.0, so this is not version skew), with TS IDL codegen already run:Fail-before (this change reverted;
bindings/web/node_modulesandbindings/proto-ts/node_moduleswiped):Pass-after (change applied, same wipe):
Before the fix the same
npm testrun had 49 of 56 test files failing (all downstream of the missing proto-ts install). Not run here: WASM builds, browser Playwright suites, and the other SDKs' builds — this change only touches the npm install lifecycle of the private web workspace root; CI remains the authority for those.Web SDK / Web Sample: not applicable — no runtime code changes; browser checklist items unaffected.
Labels
Web SDK- Changes to Web SDK (bindings/web)Checklist
Summary by CodeRabbit