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

[widgets] Add temporary messages for a fetch proxy #997

Merged
merged 8 commits into from
Nov 21, 2024
Merged

Conversation

styu
Copy link
Contributor

@styu styu commented Nov 21, 2024

This does expose the OSDK create client function on the widget client, which is nice because we deal with figuring out the URL for them when within the sandbox.

The end goal is to not have to proxy on the frontend and instead create a proxy on the server so we don't have to endlessly mirror all fetch/browser functionality, but we need this for now to show it working end to end first

* Temporary fetch proxy response
* Will be removed in favor of server side proxy
*/
export interface _unstable_FetchResponseSuccess {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i prefixed these with _unstable_ because i don't think they should be long lived

M extends HostMessage<C>,
> {
(event: CustomEvent<M["payload"]>): void;
export interface HostMessageEventListener<P extends HostMessage.Payload> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i learned these were all wrong and did not actually handle the union type correctly 🤦‍♀️

@styu styu marked this pull request as ready for review November 21, 2024 17:57
packages/widget.client.unstable/src/client.ts Outdated Show resolved Hide resolved
packages/widget.client.unstable/src/client.ts Outdated Show resolved Hide resolved
}

function serializeHeaders(headers: HeadersInit | undefined) {
if (headers === undefined || !(headers instanceof Headers)) {

Choose a reason for hiding this comment

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

Apologies I know this is copied from me, but for completeness we could also error if not an instance of Headers, at least then all of the data type assumptions of this code error loudly if the client did something we don't support

Copy link
Member

Choose a reason for hiding this comment

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

So HeadersInit can be any of the following string, string][] | Record<string, string> | Headers. But also you can pass all of those to new Headers() to ensure you have a headers object.

You could also therefore simplify serialization to just [...new Headers(headers ?? {}).entries()] and rehydrate on the other side with new Headers(...)

rosscourt
rosscourt previously approved these changes Nov 21, 2024
return new Promise<Response>((resolve, reject: (error: Error) => void) => {
function handleMessage(event: MessageEvent) {
const { data } = event;
visitHostMessage(event.data, {
Copy link
Member

Choose a reason for hiding this comment

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

:(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol i like that the compiler complains if you leave something out!

packages/widget.client.unstable/src/host.ts Outdated Show resolved Hide resolved
}

function serializeHeaders(headers: HeadersInit | undefined) {
if (headers === undefined || !(headers instanceof Headers)) {
Copy link
Member

Choose a reason for hiding this comment

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

So HeadersInit can be any of the following string, string][] | Record<string, string> | Headers. But also you can pass all of those to new Headers() to ensure you have a headers object.

You could also therefore simplify serialization to just [...new Headers(headers ?? {}).entries()] and rehydrate on the other side with new Headers(...)

@policy-bot policy-bot bot dismissed rosscourt’s stale review November 21, 2024 19:38

Invalidated by push of b260312

@@ -32,22 +32,13 @@ export function serializeRequest(
id,
url: input,
method: init?.method ?? "GET",
headers: serializeHeaders(init?.headers),
headers: Object.fromEntries([
Copy link
Member

Choose a reason for hiding this comment

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

super nit: you dont need to fromEntries here since [string, string][] is a valid HeadersInit which will work on the other side

ericanderson
ericanderson previously approved these changes Nov 21, 2024
@policy-bot policy-bot bot dismissed ericanderson’s stale review November 21, 2024 19:48

Invalidated by push of 482bc68

@styu styu merged commit 21d2dd1 into main Nov 21, 2024
7 checks passed
@styu styu deleted the syu/fetch-proxy branch November 21, 2024 20:06
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.

3 participants