Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the repository to a pnpm workspace, updating numerous .gitignore and CONTRIBUTING.md files, adjusting CI scripts, and updating generated protobuf files. Feedback on these changes highlights several improvement opportunities: nested npm scripts in gax/package.json should be updated to pnpm, stubbed tests in pack-n-play should be explicitly skipped using it.skip rather than left empty, and relative file: paths in package dependencies should be replaced with the workspace: protocol. Additionally, the reviewer advises against using 'as any' type assertions in apirequest.ts and hapi.ts, recommending instead that workspace dependency versions be aligned or safer type casting be implemented to preserve TypeScript's type safety.
| .setUrl(urlString) | ||
| .setUserAgent(getSingleHeader(req!.headers['user-agent'])) | ||
| .setReferrer(getSingleHeader(req!.headers.referrer)) | ||
| .setUserAgent(getSingleHeader(req!.headers['user-agent'] as any)) | ||
| .setReferrer(getSingleHeader(req!.headers.referrer as any)) | ||
| .setStatusCode(attemptToExtractStatusCode(req!)) |
There was a problem hiding this comment.
Using as any here discards type safety. If the type mismatch is due to req.headers being typed as IncomingHttpHeaders (where header values can be string | string[] | undefined) and getSingleHeader expecting a specific type, it is safer to cast to the expected type (e.g., string | string[] or string) or handle the undefined case explicitly, rather than using as any.
…ng minimatch in c8
…pendencies are installed
This reverts commit 37e6bcf.
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
…emove c8 text reporter
…in conformance scripts
# Conflicts: # .github/workflows/bigtable-conformance.yaml # .github/workflows/conformance-test.yaml # .github/workflows/generator-tests.yaml # .github/workflows/issues-no-repro.yaml # .github/workflows/mandatory-conformance.yaml # .github/workflows/update-api-list.yaml
1 participant
Migrate to PNPM workspace. This will speed up dependency installation, reduces disk space usage by globally hoisting packages, and provides stricter dependency resolution.
pnpm-workspace.yaml/pnpm-lock.yamlto manage all dependencies.presubmit.yamlandwindows-presubmit.yamlto usepnpm install --frozen-lockfileci/run_single_test.shuse PNPM workspace andci/run_conditional_tests.shto bypass deprecations on Node v22.npmexecution chains fromcore/packages/gax/package.json, updated pack-n-play test fixturesnpm run compilepasses with new PNPM dependency tree.package-lock.jsonfrom all.gitignorefiles as a best practice: since PNPM is now the source of truth,package-lock.jsonshould never exist. If a developer accidentally runsnpm installout of habit, the untracked lockfile will be highlighted bygit status, which will hopefully be enough to indicate the mistake.CONTRIBUTING.mdfiles to usepnpm installwhen setting up local development environments.package.jsonto enforcepnpmas the strict package manager viaonly-allowand Corepack to prevent developers from accidentally running npm install and breaking the workspace