Skip to content

feat(internal/librarian/nodejs): add esm support for google-cloud-tasks#6156

Merged
quirogas merged 41 commits into
mainfrom
feat/nodejs-esm-support
May 26, 2026
Merged

feat(internal/librarian/nodejs): add esm support for google-cloud-tasks#6156
quirogas merged 41 commits into
mainfrom
feat/nodejs-esm-support

Conversation

@quirogas
Copy link
Copy Markdown
Contributor

@quirogas quirogas commented May 23, 2026

Add support for generating ES Modules (ESM), enabling us to build google-cloud-tasks cleanly without introducing tight coupling to librarian.

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

quirogas added 30 commits May 20, 2026 22:59
…de sidekick test suites to pass TestGolangCILint
@quirogas quirogas self-assigned this May 23, 2026
Copy link
Copy Markdown
Contributor

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

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

Comment thread internal/librarian/nodejs/generate.go
Comment thread internal/librarian/nodejs/generate.go
@quirogas quirogas changed the title Feat/nodejs esm support feat(internal/librarian/nodejs): add esm support May 25, 2026
@quirogas
Copy link
Copy Markdown
Contributor Author

/gemini review

Copy link
Copy Markdown
Contributor

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

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

Comment thread internal/librarian/nodejs/generate.go
@quirogas quirogas changed the title feat(internal/librarian/nodejs): add esm support feat(internal/librarian/nodejs): add esm support for google-cloud-tasks May 26, 2026
@quirogas quirogas marked this pull request as ready for review May 26, 2026 01:23
@quirogas quirogas requested a review from a team as a code owner May 26, 2026 01:23
@JoeWang1127
Copy link
Copy Markdown
Contributor

Closes #5954

Fixes #5954

Copy link
Copy Markdown
Contributor

@JoeWang1127 JoeWang1127 left a comment

Choose a reason for hiding this comment

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

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?

@quirogas
Copy link
Copy Markdown
Contributor Author

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.

@quirogas quirogas merged commit b768f82 into main May 26, 2026
48 checks passed
@quirogas quirogas deleted the feat/nodejs-esm-support branch May 26, 2026 14:01
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.

nodejs: Fix Brittle Post-Processor Path Scanning in google-cloud-tasks

2 participants