ipfs / protons

Protocol Buffers for Node.js and the browser without eval
Other
32 stars 23 forks source link

Type name conflict when enum is named `Error` #64

Open fryorcraken opened 2 years ago

fryorcraken commented 2 years ago

Using the following protobuf definition:

message HistoryResponse {
  enum Error {
    ERROR_NONE_UNSPECIFIED = 0;
    ERROR_INVALID_CURSOR = 1;
  }
  optional Error error = 4;
}

The TypeScript generated has the following typescript error:

src/proto/store_v2beta3.ts:364:21 - error TS2351: This expression is not constructable.
  Type 'typeof Error' has no construct signatures.

364           throw new Error('Protocol error: required field "messages" was not found in object')
                        ~~~~~

src/proto/store_v2beta4.ts:373:21 - error TS2351: This expression is not constructable.
  Type 'typeof Error' has no construct signatures.

373           throw new Error('Protocol error: required field "messages" was not found in object')

This is because typescript thinks that Error refers to the enum and not to the native type Error:

export namespace HistoryResponse {
  export enum Error {
    ERROR_NONE_UNSPECIFIED = 'ERROR_NONE_UNSPECIFIED',
    ERROR_INVALID_CURSOR = 'ERROR_INVALID_CURSOR'
  }

  enum __ErrorValues {
    ERROR_NONE_UNSPECIFIED = 0,
    ERROR_INVALID_CURSOR = 1
  }

  export namespace Error {
    export const codec = () => {
      return enumeration<Error>(__ErrorValues)
    }
  }

  let _codec: Codec<HistoryResponse>

  export const codec = (): Codec<HistoryResponse> => {
    if (_codec == null) {
      _codec = message<HistoryResponse>((obj, writer, opts = {}) => {
        if (opts.lengthDelimited !== false) {
          writer.fork()
        }

        if (obj.messages != null) {
          for (const value of obj.messages) {
            writer.uint32(18)
            WakuMessage.codec().encode(value, writer)
          }
        } else {
          throw new Error('Protocol error: required field "messages" was not found in object')
        }
achingbrain commented 2 years ago

I'm not sure what a good solution to this is. The generator could have a list of built in type names that it would rename if encountered, but that might be surprising to the user?

Is it a big deal to rename the type in your .proto file since the format on the wire would be the same?

I'm open to suggestions.

tinytb commented 2 years ago

It seems reasonable to document the fact that Error is a reserved word, so the TypeScript error is expected behavior. We should also point out the workaround that you can rename the type in your .proto file, without breaking the wire format.

Caveat: I'm new to this project, so this may be completely wrong.

fryorcraken commented 2 years ago

It seems reasonable to document the fact that Error is a reserved word, so the TypeScript error is expected behavior. We should also point out the workaround that you can rename the type in your .proto file, without breaking the wire format.

Yes, this seems to be the appropriate way. Ensuring it's clearly described in readme so there is no time wasted. I would not go down the route of engineering this one. I open the issue more for doc purposes so that the next user does not end up confused by this error.