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

Add CommonJS support as a plugin option #587

Closed
mfridman opened this issue Oct 12, 2023 · 12 comments · Fixed by #648
Closed

Add CommonJS support as a plugin option #587

mfridman opened this issue Oct 12, 2023 · 12 comments · Fixed by #648

Comments

@mfridman
Copy link
Member

The current code generators (like bufbuild/protobuf-es and connectrpc/es) support ESM only. What do you think about adding an opt-in plugin option to modify the default behaviour?

As this project becomes more popular, it'd be nice for the generated code to "play nice" with the rest of the ecosystem. For example, some projects like NestJS do not work well with ESM-only. And the alternatives aren't great, such as importing the module asynchronously.

Given that the Buf Schema Registry (BSR) hosts plugins with a pre-configured set of options, it means we'd need to publish a second plugin named based on the import. This isn't great, so something to think about.

https://buf.build/bufbuild/es vs https://buf.build/bufbuild/es-commonjs (does not exist)

Lastly, since we'll want this to be a "registry-compatible" plugin, we'll need a commonjs implementation for rewriting import paths.

@therockstorm
Copy link

therockstorm commented Oct 13, 2023

From this Slack thread, without BSR support for CommonJS (and, by extension, NestJS), the "best" workaround is to bypass BSR completely and instead create a monorepo with CI builds that generate CommonJS NPM packages.

Is this true? If so, how likely is Buf to support CommonJS and would we be talking months or years? If this is farther away than a couple of months, we'll have to move ahead with doing all of this ourselves and removing BSR from our workflow. Sadly, NestJS has no plans to update to ESM.

@timostamm
Copy link
Member

Hey Rocky, I am not sure that bypassing the BSR completely is the best workaround. NestJS does not support ESM-only npm packages, but it does support TypeScript files with ESM imports.

For example, you might have an import like the following in your app:

// app.module.ts
import { AppController } from './app.controller';

This works in Nest, because all source files are transpiled to CJS.

If you generate code locally with protoc-gen-es, you can set the option target=ts to get the same result - TypeScript files with ESM imports. If you import them in your app, they will be transpiled to CJS like any other source file.

It isn't as neat as remote packages, but you can generate local code from remote protobuf files on the BSR.

You need one configuration file:

# buf.gen.yaml
version: v1
plugins:
  - plugin: buf.build/bufbuild/es
    out: src/gen
    opt: target=ts
  - plugin: buf.build/connectrpc/es
    out: src/gen
    opt: target=ts

Then you can run buf generate with any BSR module. For example:

$ buf generate buf.build/connectrpc/eliza
$ tree src/gen
src/gen
└── connectrpc
    └── eliza
        └── v1
            ├── eliza_connect.ts
            └── eliza_pb.ts

3 directories, 2 files

Your use case might vary, but right now, this would be my recommended approach with the BSR in case the framework does not support ESM.

@timostamm
Copy link
Member

Lastly, since we'll want this to be a "registry-compatible" plugin, we'll need a commonjs implementation for rewriting import paths.

This should not be necessary. Import statements are generated by @bufbuild/protoplugin here:

if (typeOnly) {
c.push(`import type ${what} from ${literalString(from)};\n`);
} else {
c.push(`import ${what} from ${literalString(from)};\n`);

This could easily be extended to support CommonJS require() calls with a plugin option. The logic to rewrite import paths would continue to work as is.

The more challenging part are export statements. They are currently handwritten by plugins. In order to support CommonJS with a plugin option, we should provide an API to generate export statements, which automatically produces a ECMAScript export statement or a CommentJS export assignment, based on the plugin option.

@therockstorm
Copy link

therockstorm commented Oct 13, 2023

@timostamm Thanks for the reply! Our use case:

We define EventBridge message schemas with Protobuf. Each message is sent from a single producer microservice to one or many consumer microservices. Each microservice is in a different GitHub repository.

The ideal flow is as follows:

  1. In each producer repo, define Protobuf schemas and publish them to BSR
  2. In each consumer repo, pull in BSR-generated assets for each producer you handle messages from and handle the messages

Since we can't use the BSR-generated assets, however, the flow is:

  1. Same as above
  2. In each consumer repo, run buf generate buf.build/[PRODUCER_REPO]:[SOME_REF] --include-imports to generate the producer's proto files and handle messages

This works fine if a consumer is handling messages from a single producer. With more than one, however, we run into dependency management issues. Say we have two producers, P1 and P2. Both of their schemas depend on different versions of a dependency.

Assuming we're generating these files into the same location, say src/gen, we need to ensure all of P1 and P2's dependencies are using the same versions or, at the very least, there aren't breaking changes between dependency versions. An alternative is to create a buf.gen.yaml file for each producer with different out directories, complicating the configuration and likely leading to custom tooling.

This solution also means every consumer is regenerating these files on each CI build with possibly differing versions of @bufbuild/protoc-gen-es and @bufbuild/buf, opening the door to hard to diagnose bugs that work fine in some repos but not others.

The above seems like a high price to pay just to keep using BSR as it provides minimal benefit for this use case. Instead, it seems less error prone and produces less cognitive load for microservice developers to move Protobuf schemas to a monorepo that generates the NPM packages. This gets us pretty close to the "ideal flow" above with the following downsides:

  1. Custom tooling we have to build and maintain (though, as described above, we'd have some custom tooling anyway unless BSR adds CommonJS support)
  2. The Protobuf schemas are no longer colocated with producer code, making iteration slower, which we could solve with yet more custom tooling

@timostamm
Copy link
Member

Thanks for sharing the details on the use case, @therockstorm.

You can re-use buf.gen.yaml files with the --template and --output flags. But I'd be the first to agree that remote packages are a better fit 🙂

We have a couple of other features in the pipeline, but this will be implemented. It will most likely land before the end of the year.

@timostamm timostamm changed the title Consider adding CommonJS support as an option Add CommonJS support as a plugin option Oct 16, 2023
@therockstorm
Copy link

therockstorm commented Oct 16, 2023

That's excellent news, @timostamm! I was using https://github.com/timostamm/protobuf-ts prior to https://github.com/bufbuild/protobuf-es and they, along with the rest of the Buf tooling, are excellent to use. Thanks to the team for their efforts and for your helpful, timely responses on this issue.

@krapie
Copy link

krapie commented Dec 12, 2023

Wow, I really needed this option! Thanks!
I hope updated protobuf-es to be released ASAP :)

@smaye81
Copy link
Member

smaye81 commented Dec 12, 2023

Hi @krapie we just published v1.6.0 to npm: https://www.npmjs.com/package/@bufbuild/protobuf

@krapie
Copy link

krapie commented Dec 13, 2023

@smaye81 Thank you for the release! But I'm still wondering why my XXX_connect.js is still using export statement, which causes ts-node SyntaxError: Unexpected token 'export' error.

Screenshot 2023-12-13 at 11 42 19 AM

@timostamm
Copy link
Member

@krapie, protoc-gen-connect-es wasn't updated to support the option yet. But if you update to the just released v1.2.0, it should generate CommonJS exports too:

version: v1
plugins:
  # You'll need @bufbuild/protoc-gen-es v1.6.0 or later, 
  # and @bufbuild/protoc-gen-connect-es v1.2.0 or later
  - plugin: es
    out: src/gen
    opt: js_import_style=legacy_commonjs
  - plugin: connect-es
    out: src/gen
    opt: js_import_style=legacy_commonjs

We don't have integration tests for this plugin option, but it should do the job. Let us know if you encounter any problems 🙂

@krapie
Copy link

krapie commented Dec 14, 2023

@timostamm Ohh, I thought this release updates all plugins to provide new option (I was able to use js_import_style=legacy_commonjs option on connect-es) 😄

It is now working perfectly. Thank you for your fast feedback and release!

@timostamm
Copy link
Member

Just to provide context:

All plugins based on the plugin framework @bufbuild/protoplugin get the new option. protoc-gen-es, protoc-gen-connect-es, and protoc-gen-connect-query are all based on it.

But the plugin framework can only modify export statements for CommonJS when it knows what the export statements are. For example, protoc-gen-connect-es was using this call to generate a service:

f.print("export const ", localName(service), " = {");

"export const" is just an opaque string to the plugin framework, and it would only be possible to modify it safely with a full parser, which is a big perf hit for large protobuf modules. So the latest release of the plugin framework adds a new method to generate export statements, and the latest release of protoc-gen-connect-es uses it:

f.print(f.exportDecl("const", localName(service)), " = {");

Similar to the already existing f.import(), this method generates a statement conditionally. Plugins must use f.import() and f.exportDecl() to fully support the new plugin option. More information is available in the PR, and in the documentation.

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 a pull request may close this issue.

5 participants