V2: Remove keep_empty_files option and change default to always generate

[srikrsna-buf]

With v2 we are also generating the service descriptors, and file descriptor with any file options. The likelihood of an empty file with no top level declarations is minuscule. So we can remove the keep_empty_files option and change the default to always generated a file.

For most users this is a seamless change, they may see a extra files generated. For users that are using the option they will have to remove it and will get the same behavior.

[timostamm]

What about users of @bufbuild/protoplugin?

In a custom plugin, you're most likely only generating a subset of files. The most common use case is generating something for services.

We've made it very easy to do the right thing by default in such a plugin: Don't generate empty files, but still include a preamble in the generated files. Users still only need to loop through services.

With this change, that's no longer the case. In the plugin example, we're generating an extra file in packages/protoplugin-example/src/gen/customoptions/default_host_twirp.ts that is effectively empty. That's not ideal.

One option would be for plugin authors to only call f.preamble if they intend for the file to be generated. I'd much prefer if we can avoid this, because it requires an extra step that's easy to forget or get wrong.

WDYT?

[timostamm]

Users who set this option in their buf.gen.yaml will get an error with this change, and they'll wonder why. Can we provide a helpful error message in this case? I guess that's a bit tricky, because we'll no longer support this option, but plugin users can't use it either if we error on it.

[srikrsna-buf]

I think this should be handled as part of the upgrade guide.

[srikrsna-buf]

One option would be for plugin authors to only call f.preamble if they intend for the file to be generated. I'd much prefer if we can avoid this, because it requires an extra step that's easy to forget or get wrong.

Isn't this true even if they have this option?

[timostamm]

One option would be for plugin authors to only call f.preamble if they intend for the file to be generated. I'd much prefer if we can avoid this, because it requires an extra step that's easy to forget or get wrong.

Isn't this true even if they have this option?

The preamble has special behavior, see last paragraph:

  /**
   * Create a standard preamble the includes comments at the top of the
   * protobuf source file (like a license header), as well as information
   * about the code generator and its version.
   *
   * The preamble is always placed at the very top of the generated file,
   * above import statements.
   *
   * A file with a preamble but no other content is still considered empty,
   * and will not be generated unless the plugin option keep_empty_files=true
   * is set.
   */
  preamble(file: DescFile): void;

Meaning that it's fine to write a plugin like this:

function generateTs(schema: Schema) {
  for (const file of schema.files) {
    const f = schema.generateFile(file.name + "_foo.ts");
    f.preamble(file);
    for (const service of file.services) {
      if (hasOption(service, foo_bar)) {
        f.print("// service", service.typeName);
      }
    }
  }
}

Without this feature, users can't loop over file.services, and have to do something like this:

function generateTs(schema: Schema) {
  for (const file of schema.files) {
    const f = schema.generateFile(file.name + "_foo.ts");
    const services = file.services.filter(s => hasOption(s, foo_bar));
    if (services.length > 0) {
      f.preamble(file);
      for (const service of services) {
        f.print("// service", service.typeName);
      }
    }
  }
}

Since a service generator is probably the most common use case for a plugin, I'd prefer to retain this feature.