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

URI generics default to Uri; ConfigBuilder accepts string | Uri where possible #81

Open
wants to merge 5 commits into
base: origin-dev
Choose a base branch
from

Conversation

krisbitney
Copy link
Contributor

@krisbitney krisbitney commented Sep 14, 2023

Generic URI types in client are now a union type Uri | string

Closes #79

@krisbitney krisbitney changed the title URI generics default to Uri; ConfigBuilder accepts string | Uri wherepossible URI generics default to Uri; ConfigBuilder accepts string | Uri where possible Sep 14, 2023
@@ -118,7 +115,7 @@ export class PolywrapClient extends PolywrapCoreClient {
}

@Tracer.traceMethod("PolywrapClient: invoke")
public async invoke<TData = unknown, TUri extends Uri | string = string>(
public async invoke<TData = unknown, TUri extends Uri | string = Uri>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the default to Uri doesn't fix #79 , it just flips the coin and requires you to have a Uri instance if you specify a return type.

The problem is that if you specialize the generic with the first template argument, the second one appears to be required. Instead of using a template argument for TUri maybe we should just make the actual argument type options: InvokerOptions<Uri | string> and we can determine the type within the function's body. This way if you specialize the first template argument, there isn't a second one that needs to be specified as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, my mistake. I misunderstood. I read "...instead of String we should support that the default is Uri" and thought we just wanted to modify the generic type. I changed everything to use union types instead, so we should be good to go.

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 this pull request may close these issues.

JS Client: Uri inference in invocation
2 participants