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

Add fetch instance option #424

Closed
wants to merge 3 commits into from
Closed

Conversation

chanced
Copy link

@chanced chanced commented Jan 19, 2023

This fixes #403 by adding fetch as an option to GrpcWebTransportOptions so that users of SvelteKit gain various benefits from providing a fetch instance (docs).

My prettier config mangled the formatting. Is there anyway you could please share your prettier config so that I can align it?

edit:

This was the easiest path forward. Arguably, it would be slightly better performance wise to add fetch as an optional parameter to the returned functions. However, doing so would require a bit more changes, including the generators.

I'm willing to make those changes if that's the path you'd prefer.

@CLAassistant
Copy link

CLAassistant commented Jan 19, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@chanced chanced changed the title Adds fetch instance option (#403) Adds fetch instance option Jan 19, 2023
@chanced chanced force-pushed the add-optional-fetch branch from 7a049cb to 7ef2ee2 Compare January 19, 2023 22:48
@timostamm
Copy link
Member

Apologies for the late reply, @chanced. It looks like an option to provide fetch in the transport is the best way forward.
Having to create a new transport instance for every server side render is not ideal, but I don't think it matters much in the real world.

The pattern could be to define a function for creating clients with the desired transport at a single place in the app:

export function createClient<T extends ServiceType>(service: T, fetch?: FetchFn): PromiseClient<T> {
  const t = createConnectTransport({baseUrl: "/", fetch});
  return createPromiseClient(service, t);
}

And than use it in the SvelteKit load functions:

export async function load({ fetch, params }) {
  const c = createClient(SomeService, fetch);
  return await c.someMethod({id: params.id});
}

Some initial testing by @paul-sachs looked promising, but there are issues with binary data and we did not look into server-streaming yet. Supporting SSR via a custom fetch sounds great, but we need more investigation and also integration tests to make sure it works properly.

@bufdev bufdev changed the title Adds fetch instance option Add fetch instance option Apr 4, 2023
@timostamm
Copy link
Member

Superseded by #618.

Thank you for the contribution, @chanced!

@timostamm timostamm closed this May 17, 2023
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.

Allow customizing fetch
3 participants