feat(internal/librarian/nodejs): add esm support for google-cloud-tasks#6156
Conversation
…bal enable dependency
…andardizing on pnpm
… to preserve package names and baselines
… Node APIs during migration
…de sidekick test suites to pass TestGolangCILint
… into feat/use-pnpm-install
There was a problem hiding this comment.
Code Review
This pull request introduces support for ES Modules (ESM) in Node.js generation by adding an esm configuration flag and updating the generation and post-processing logic to handle ESM-specific flags and directory structures. Review feedback indicates that the ESM implementation is currently incomplete, as several functions still use hardcoded paths that do not account for the ESM directory structure. Additionally, a simplification for the argument construction logic in the proto compilation step was suggested.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for generating ES Modules (ESM) outputs for Node.js packages. This includes updating the configuration schema, merging logic, generator arguments, and post-processing steps to handle ESM. A test case is also added to verify the ESM generator arguments. The reviewer identified a critical issue where the post-processor functions copyMissingProtos and restoreCopyrightYear still hardcode the src directory, which will fail for ESM libraries that use esm/src. The reviewer recommends deriving relative paths before the post-processing phase to resolve this.
JoeWang1127
left a comment
There was a problem hiding this comment.
Could you add more context on what is a ES Module and why, without this change, the generation of google-cloud-task will introduce coupling in librarian?
Yes, my previous message was a bit confusing. All packages except google-cloud-tasks use the CommonJS module system. I believe it stemmed from an experiment. To generate we only have to support ESM for tasks, I want to avoid implementing a overly complex solution that could create tightly coupled code we might not want to support long-term. The current implementation will generate ESM code without adding support for future ESM libraries. Once this PR is merged, we should consult with the Node.js team to better understand their roadmap for ESM. |
Add support for generating ES Modules (ESM), enabling us to build
google-cloud-taskscleanly without introducing tight coupling tolibrarian.We should investigate the plans for both this package and ESM support in the future. To prevent introducing broad codebase coupling or inheriting potential tech debt for an ESM standard the Node.js team may choose not to adopt in the google-cloud-node monorepo. In the meantime this changes will add a minimal set of changes that allows us to generate
google-cloud-tasks, but will not add full support for other ESM packages.Closes #5954