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

ConnectTransportOptions and similar types should be exported for TypeScript users #549

Closed
stabai opened this issue Mar 29, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@stabai
Copy link

stabai commented Mar 29, 2023

Is your feature request related to a problem? Please describe.
I would like to create a factory method to automatically create the Transport object for my service clients, but I don't have a reasonable type to return from my method. Not specifying a return type makes TypeScript upset, because it sees my httpVersion as string rather than '1.1'|'2'.

Describe the solution you'd like
Export the *TransportOptions types:

Please specify whether the request is for Connect for Web or Connect for Node.js
Node.js

Describe alternatives you've considered
My workaround was to just define my own custom compatible type to return.

Additional context
N/A

@stabai stabai added the enhancement New feature or request label Mar 29, 2023
@stabai stabai changed the title Type ConnectTransportOptions should be exported for TypeScript users ConnectTransportOptions and similar types should be exported for TypeScript users Mar 29, 2023
@timostamm
Copy link
Member

Thanks for raising this, Shawn.

We're still pre v1, and we might want to put a more convenient createClient() function on top that simply lets you choose the protocol via an option instead of separate functions (bundle size is not a great concern for Node.js). We are also considering generating more convenient client constructors, and other options before we cut a v1. Because of those possible additions, we don't want to export the option types at this time. We would not want to remove them once they are released, and then they would unnecessarily clutter the exports post v1.

But there is a simple solution for your use case:

const transportOptions = {
  baseUrl: "https://demo.connect.build",
  httpVersion: "2" as const,
};

const transport = createConnectTransport(transportOptions);

The as const tells TypeScript to treat the value as a literal type "2" instead of a string, so that transportOptions satisfies the option type.

@timostamm
Copy link
Member

We're a bit hesitant to export the option types at this point.

There are subtle differences between the options between the @connectrpc/connect-web and @connectrpc/connect-node packages. I think we are better off nudging the types to be consistent, and export just a single common type for all of them. This will be more complicated to pull off if we export them now.

@stabai
Copy link
Author

stabai commented Sep 1, 2023

That seems like a fine compromise to me. 🙂

For posterity, here is my dissenting opinion: I think there are lots of cases where indirection is desirable when building configuration options, and the specific implementation can be an important part of that. For example, our ORM has a set of generic options, and also a set of server-specific options (we use SQLite for unit tests and PostgreSQL at runtime). We merge different portions of options together several time to build the right configuration (e.g. our base options => our base postgres options => our postgres options for production). Having all of these types makes it easier for us to do this while also ensuring that the options are correctly formed. The same thing would likely be true for our transport factory (e.g. base options => grpc options => specific service options), so it would be nice to have the types exported.

All of this being said, I don't fully understand the situation, so feel free to ignore me. 🙃 My position is that semver states that even minor subversions of v0.X are never required to be backward-compatible, so that means you can change whatever you like. That being said, some people may be unhappy with that perspective. 😅

@timostamm
Copy link
Member

We never came around to consolidate transport options. All option types for transports are exported now, with #959.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants