Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate grpc/pb with commonjs_strict #5336

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

niloc132
Copy link
Member

@niloc132 niloc132 commented Apr 5, 2024

This enables applications to load more than one version of the JS API as ES6 modules, without the implementations interfering with each other. Unfortunately, the JS protoc plugin doesn't support this case well, so workarounds are required to correctly reference proto files that use packages.

Fixes #5318

@niloc132 niloc132 requested a review from mattrunyon April 9, 2024 15:03
@mattrunyon
Copy link
Contributor

I just pulled and built this (w/ prepareCompose) and see a proto.google.protobuf.Timestamp in global still. Seems that's the only thing on the global proto at least

@niloc132
Copy link
Member Author

Thanks - that's part of the root bug, that even the built-in protoc-js-gen doesn't correctly support commonjs_strict for its own generated files. See protocolbuffers/protobuf-javascript#25.

The good news is that we don't actually use that package or its message types at all presently, so we can't run into this bug with them. If that changes, we're going to need to entirely give up on the existing codegen.

Copy link
Contributor

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

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

Checked locally and the only thing in global now is proto.google.protobuf.Timestamp, but as @niloc132 noted we don't use this.

Ran web e2e tests against this branch and they passed.

Didn't check that the TS gen worked properly, so if that works then this looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS API protobuf pollutes global namespace
2 participants