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

feat(extension): add sapi_ipc_send_chunk and sapi_ipc_send_event #679

Merged
merged 5 commits into from Oct 12, 2023

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Oct 10, 2023

⚠️ Only MacOS and iOS are supported by this PR. I don't have time to implement the other platforms at the moment.

  • Before calling sapi_ipc_send_chunk, you must first set the Transfer-Encoding header to chunked and then call sapi_ipc_reply.
  • When using sapi_ipc_send_chunk, the frontend should use XMLHttpRequest. Currently, the Fetch API does not support response streaming in WebKit.
  • Before calling sapi_ipc_send_event, you must first set the Content-Type header to text/event-stream and then call sapi_ipc_reply.
  • When using sapi_ipc_send_event, the frontend should use EventSource.

- Before calling sapi_ipc_send_chunk, you must set the Transfer-Encoding header to `chunked` and then call sapi_ipc_reply.
- When using sapi_ipc_send_chunk, the frontend should use XMLHttpRequest. Currently, the Fetch API does not support response streaming in WebKit.
- Before calling sapi_ipc_send_event, you must set the Content-Type header to `text/event-stream` and then call sapi_ipc_reply.
- When using sapi_ipc_send_event, the frontend should use EventSource.
Copy link
Member

@jwerle jwerle left a comment

Choose a reason for hiding this comment

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

Left some comments. We'll need to set isHTTP = true in src/window/win.cc too. I wonder if we need a more general way to say where the source of the IPC::Message is from cc @chicoxyzzy

include/socket/extension.h Show resolved Hide resolved
@@ -1096,6 +1096,51 @@ extern "C" {
const sapi_json_any_t* json
);

/**
* Write a chunk to the HTTP response associated with the IPC result.
Copy link
Member

Choose a reason for hiding this comment

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

Can we include a note about this working on iOS/macOS only?

Copy link
Member

Choose a reason for hiding this comment

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

or warning if platform is not iOS/macOS, but I'm not sure if that check is available here

@jwerle jwerle added the enhancement New feature or request label Oct 10, 2023
@aleclarson
Copy link
Contributor Author

👍

@jwerle jwerle self-requested a review October 10, 2023 23:23
Copy link
Member

@chicoxyzzy chicoxyzzy left a comment

Choose a reason for hiding this comment

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

We have to check that this change doesn't break other platforms. Should we just allow CI for external PRs for now?

@@ -1096,6 +1096,51 @@ extern "C" {
const sapi_json_any_t* json
);

/**
* Write a chunk to the HTTP response associated with the IPC result.
Copy link
Member

Choose a reason for hiding this comment

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

or warning if platform is not iOS/macOS, but I'm not sure if that check is available here

message.buffer.bytes,
message.buffer.size
);
auto msg = SSC::IPC::Message(message);
Copy link
Member

Choose a reason for hiding this comment

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

We wanted an AOT decode so the sapi_ipc_message_get() calls get a decoded value

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've guaranteed that Router::invoke receives an IPC::Message with decoded values in other parts of this PR, which means the values will be decoded by the time the commented line is reached.

https://github.com/aleclarson/socket/blob/b9449443918685f6b2d15cca18ad62efd182ed06/src/ipc/bridge.cc#L2728-L2736

When Router::invoke is called with a uri string, values are always decoded (is this a problem?). When Router::invoke is called with a IPC::Message instead, it's assumed that the message has decoded values, which I've checked is true anywhere this overload is used as of now.

Comment on lines +658 to +674
if (strcasecmp(name, "content-type") == 0 &&
strcasecmp(value, "text/event-stream") == 0) {
result->post = SSC::Post();
result->post.event_stream =
std::make_shared<std::function<bool(const char*, const char*, bool)>>(
[result](const char* name, const char* data, bool finished) {
return false;
});
} else if (strcasecmp(name, "transfer-encoding") == 0 &&
strcasecmp(value, "chunked") == 0) {
result->post = SSC::Post();
result->post.chunk_stream =
std::make_shared<std::function<bool(const char*, size_t, bool)>>(
[result](const char* chunk, size_t chunk_size, bool finished) {
return false;
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could probably move this to sapi_ipc_reply later as a perf optimization.

@jwerle jwerle merged commit 32b8727 into socketsupply:master Oct 12, 2023
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

Successfully merging this pull request may close these issues.

None yet

3 participants