Skip to content

chore(generator): move gapic-generator-typescript off Bazel into the root pnpm workspace - #9381

Closed
bshaffer wants to merge 18 commits into
mainfrom
consolidate-pnpm-lockfiles-pnpm-10
Closed

bshaffer wants to merge 18 commits into
mainfrom
consolidate-pnpm-lockfiles-pnpm-10

Conversation

@bshaffer

@bshaffer bshaffer commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Split into reviewable pieces. Land these first, then merge main here and this PR shrinks to just the generator change:

What's left here

The Bazel WORKSPACE contained npm_translate_lock(pnpm_lock = "//:pnpm-lock.yaml"), hard-wiring Bazel to the generator's nested lockfile. So removing Bazel and moving the generator into the root workspace cannot be separated — they're one change.

Hand-written surface is ~150 lines:

File What
pnpm-workspace.yaml add the generator as a member
typescript/src/generator.ts protobufjs v7/v8 cast bridge + rationale
package.json drop @bazel/bazelisk and a dead npm-style overrides
.github/workflows/generator-tests.yaml bazelisk → pnpm + protoc

Everything else is generated-file deletion (WORKSPACE, BUILD.bazel, MODULE.bazel, .bazelrc, package-lock.json, the nested pnpm-lock.yaml) plus the root lockfile update. Reviewers: filter out *lock* and this is 4 files.

On the protobufjs casts

proto3-json-serializer@4 (latest) hard-depends on protobufjs@^7; the generator wants ^8. Hoisting into the root workspace loses the nested pnpm.overrides that used to unify them, so TypeScript now sees two protobufjs type trees.

Fixing this with a root pnpm.overrides entry does not work — it was tried in 2cc19a6 and broke all 18 units shards, because it forces protobufjs@8 onto google-gax, which needs v7. The casts are the correct fix until upstream supports v8. Documented inline so nobody retries the override.

Known follow-ups (not addressed here)

  1. The Bazel removal is incomplete: .bazeliskrc, repositories.bzl (loaded by the deleted WORKSPACE), and rules_typescript_gapic/ survive unreferenced.
  2. Four steps in generator-tests.yaml (lines ~159-180) are gated on steps.generator-changes.outputs.changes, but no step defines id: generator-changes — they have never run. They also use generator/... paths missing the core/ prefix. These are the combined-library Speech/Tasks tests, and the only consumer of rules_typescript_gapic/ from (1).

Needs a call on whether those tests should be revived or deleted.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request removes Bazel build configuration files and integrates the gapic-generator-typescript package directly into the pnpm workspace. It also updates TypeScript type assertions in generator.ts to resolve compilation issues and modifies the installation command in librarian.yaml. Feedback suggests removing the --force flag from the pnpm install command in librarian.yaml to prevent unnecessary slowdowns in the build pipeline.

Comment thread librarian.yaml Outdated
…files-pnpm-10

# Conflicts:
#	core/generator/gapic-generator-typescript/WORKSPACE
#	core/generator/gapic-generator-typescript/package.json
#	core/generator/gapic-generator-typescript/pnpm-lock.yaml
@bshaffer bshaffer changed the title chore: consolidate nested pnpm-lock.yaml files into root workspace and upgrade generator/librarian to pnpm 10 chore: consolidate nested pnpm-lock.yaml and upgrade generator/librarian to pnpm 10 Sep 21, 2026
@bshaffer bshaffer changed the title chore: consolidate nested pnpm-lock.yaml and upgrade generator/librarian to pnpm 10 chore: consolidate nested pnpm-lock.yaml Sep 21, 2026
@bshaffer
bshaffer marked this pull request as ready for review September 22, 2026 00:25
@bshaffer
bshaffer requested a review from a team as a code owner September 22, 2026 00:25
- generator-tests/update-api-list: --filter instead of full-workspace installs; sha256-verify protoc; deref symlinks in node_modules artifact
- document why librarian.yaml needs --no-frozen-lockfile and how to undo it after the next generator tag
- centralize the protobufjs v7/v8 casts behind one documented alias; drop dead npm-style overrides
@bshaffer bshaffer changed the title chore: consolidate nested pnpm-lock.yaml chore(generator): move gapic-generator-typescript off Bazel into the root pnpm workspace Sep 22, 2026
…10' into consolidate-pnpm-lockfiles-pnpm-10

# Conflicts:
#	.github/workflows/generator-tests.yaml
#	core/generator/gapic-generator-typescript/package.json
#	core/generator/gapic-generator-typescript/typescript/src/generator.ts
@bshaffer

Copy link
Copy Markdown
Contributor Author

Closing. This PR has been split into two reviewable pieces, and its one remaining idea was dropped on purpose.

#9413 generator: remove Bazel, build with pnpm (generator stays standalone)
#9412 CI: finish the move to pnpm 10 (generation_check.yaml, update-api-list.yaml, librarian.yaml, dead lockfiles)

Dropped: moving the generator into the root pnpm workspace.

  1. Librarian builds the released generator from a pinned tarball with --ignore-workspace (src_dir is a path inside the fetched archive). Workspace membership buys the production path nothing.
  2. Workspace membership is the sole cause of the --no-frozen-lockfile reproducibility regression. Standalone + a 9.0 lockfile means --frozen-lockfile keeps working.
  3. It costs type safety. In the root workspace, proto3-json-serializer@4 pins protobufjs@^7 while the generator needs ^8, and there is no per-project override in pnpm 10 — so generator.ts needs casts. A root pnpm.overrides.protobufjs is not an option: 2cc19a6 tried it and broke all 18 units shards (it forces protobufjs@8 onto google-gax, which needs v7); reverted in 05c2a62.
  4. It silently changes CI to test the generator against local sources instead of the published packages librarian actually installs.

@bshaffer bshaffer closed this Sep 22, 2026
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.

1 participant