Missing RpcError in exports.js (among other classes), inconsistent with index.d.ts

[roemba]

A relatively simple issue but I think quite important:
The index.d.ts contains definitions like the following (grpc-web = 1.3.0):

  export class UnaryResponse<REQ, RESP> {
    getResponseMessage(): RESP;
    getMetadata(): Metadata;
    getMethodDescriptor(): MethodDescriptor<REQ, RESP>;
    getStatus(): Status;
  }
  export class GrpcWebClientBase extends AbstractClientBase {
    constructor(options?: GrpcWebClientBaseOptions);
  }

  export class RpcError extends Error {
    constructor(code: StatusCode, message: string, metadata: Metadata);
    code: StatusCode;
    metadata: Metadata;
  }

There is nothing wrong with these definitions and they are very useful. However, exporting a class implies that I must be able to extend them, e.g. the following must work (and indeed compiles fine in Typescript):

class GRPCCallbackError extends RpcError {
  callId: string

  constructor (code: StatusCode, message: string, metadata: Metadata, callId: string) {
    super(code, message, metadata)
    this.callId = callId
  }
}

However, running this code throws an TypeError: Class extends value undefined is not a constructor or null because index.js of grpc-web does not actually export the RpcError class (or the UnaryResponse class for that matter). I confirmed this by checking grpc-web/packages/grpc-web/exports.js where only GrpcWebClientBase, StatusCode, MethodDescriptor and MethodType are exported by grpc-web.

Suggested fix:
Either change the index.d.ts to reflect that some classes are not exported or change exports.js to also export the classes that are exported in index.d.ts. I prefer the latter as that makes grpc-web more extendable.

If you think this is a good idea, I can create a merge-request for it in the coming week.

[sampajano]

Thanks so much for the detailed explanation and report! This makes a lot of sense!

Totally agreed that we should export both classes and public methods to match index.d.ts definitions!

I think #1160 is addressing a similar issue too..

Thanks so much! :):)

[roemba]

Great that you agree! I see that somebody already made a merge request which is awesome 👍

[sampajano]

Yes of course! :)

(and I've mistaken the author for you actually until you mentioned.. 😂)

Note that the PR isn't adding all missing classes as you've suggested. But maybe it's added enough of the common ones to be sufficient for most? Please feel free to comment on the PR if you have other thoughts.. :)