Skip to content

fix(generator): add long dependency to CJS template and update baselines - #8685

Merged
feywind merged 3 commits into
googleapis:mainfrom
suztomo:fix-generator-template-long
Jul 9, 2026
Merged

fix(generator): add long dependency to CJS template and update baselines#8685
feywind merged 3 commits into
googleapis:mainfrom
suztomo:fix-generator-template-long

Conversation

@suztomo

@suztomo suztomo commented Jun 18, 2026

Copy link
Copy Markdown
Member

This PR adds the missing "long" dependency to the CommonJS package.json template in gapic-generator-typescript, and updates the baselines to match.

Why long is in devDependencies

long is only required during the compilation phase of the library itself to resolve type definitions in the generated protos/protos.d.ts (which has import Long = require("long");). It is not directly imported in the runtime JavaScript code. At runtime, long is transitively resolved via google-gax -> protobufjs, so it does not need to be a production dependency of the generated library.

GAX

It also updates the google-gax dependency version to ^5.0.0 in the template to match the recently introduced google-maps-mapmanagement package:
https://github.com/googleapis/google-cloud-node/blob/6f9710c504c5272dfb59a77c7da98235ed9cf3ff/packages/google-maps-mapmanagement/package.json. The latest (non-RC) GAX version is 5.0.7: https://www.npmjs.com/package/google-gax?activeTab=versions.

Last week, I ran generate -all in #8672 and package.json was not updated. Package.json file in the keep list

- package.json
. The effect is only to upcoming newly generated libraries.

@suztomo
suztomo requested a review from a team as a code owner June 18, 2026 18:05

@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 adds the long package to devDependencies in multiple baseline and template package.json files. The reviewer correctly points out that since long is used at runtime by the generated GAPIC code, it must be declared under dependencies rather than devDependencies to avoid runtime MODULE_NOT_FOUND errors for consumers.

@quirogas quirogas added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 22, 2026
@quirogas
quirogas self-requested a review June 22, 2026 15:30

@quirogas quirogas 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.

Reverting to 5.0.0 could introduce a significant number of changes to the generated code, as google-gax provides a transitive dependency used by protobufjs during client generation. Let's talk about this change before we merge it.

@shivanee-p
shivanee-p self-requested a review June 22, 2026 21:13
@suztomo
suztomo marked this pull request as draft July 1, 2026 15:19
@suztomo

suztomo commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

@quirogas For google-gax version I found that the latest version 5.0.x versions are descendants (having the changes) of 5.1.1-rc.1. See b/527420531#comment3. Would you confirm this address your concern and remove do-not-merge label?

@quirogas

quirogas commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

@quirogas For google-gax version I found that the latest version 5.0.x versions are descendants (having the changes) of 5.1.1-rc.1. See b/527420531#comment3. Would you confirm this address your concern and remove do-not-merge label?

Thank you so much for all your work! Yes, I agree that we should be good to remove the labels. However, could you perform one final sanity check before merging the PR and review the generated changes to ensure they won't cause any issues? Feel free to send me the generated branch, and I can help look them over.

@pearigee WDYT?

@quirogas quirogas removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 1, 2026
@suztomo

suztomo commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

@quirogas With core/generator/gapic-generator-typescript/templates/cjs/typescript_gapic/package.json, this pull request only changes the package.json in a new library. This does not change the dependencies of gapic-generator-typescript.

@suztomo
suztomo marked this pull request as ready for review July 1, 2026 19:28

@feywind feywind 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.

Thanks for throwing in the gax version fix too.

@feywind

feywind commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@quirogas For google-gax version I found that the latest version 5.0.x versions are descendants (having the changes) of 5.1.1-rc.1. See b/527420531#comment3. Would you confirm this address your concern and remove do-not-merge label?

I had a PR to fix that, a while back, but it didn't end up getting merged for unrelated reasons, so I'm glad to see it in here. The rc is much older than the 5.0.x releases. Our plan is eventually to just bump gax to 5.2.x so we can avoid future weirdness with the rc.

@feywind
feywind merged commit 8e51888 into googleapis:main Jul 9, 2026
44 checks passed
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.

3 participants