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: add binary output port on HttpCallNode #392

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/app/src/components/RenderDataValue.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,14 @@ const scalarRenderers: {
</div>
);
},
binary: ({ value }) => <>Binary (length {value.value.length.toLocaleString()})</>,
binary: ({ value }) => {
// FIXME: Coercing `value.value` into a `Uint8Array` here because `Uint8Array` gets parsed as an
// object of shape `{ [index: number]: number }` when stringified via `JSON.stringify()`.
// Consider coercing it back to `Uint8Array` at the entrypoints of the boundaries between
// browser and node.js instead.
const coercedValue = new Uint8Array(Object.values(value.value));
return <>Binary (length {coercedValue.length.toLocaleString()})</>;
},
Comment on lines -208 to +215
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes an issue where hovering on the new binary output port causes the app to crash:

CleanShot 2024-04-27 at 19 50 15@2x

It crashes because value.value is a POJO instead of a Uint8Array. For example, Uint8Array(2) [123, 45] would end up being { "0": 123, "1": 45 } here.

I suspect this is because Uint8Array is serialised as such a POJO when stringified with JSON.stringify()—as is required when passing the data over to the Node.js executor. This can be verified with:

const binaryOnBrowser = new Uint8Array([123,45]); // Uint8Array(2) [123, 45]
const binaryInTransit = JSON.stringify(binaryOnBrowser);
const binaryOnNode = JSON.parse(binaryInTransit); // { "0": 123, "1": 45 }

There's probably a better place to put this coercion fix but I'm not sure where. 😬 Need the help of the maintainers on this one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, binary values don't get transmitted right across the remote debugger protocol. Definitely something that needs to be fixed at some point.

audio: ({ value }) => {
const {
value: { data },
Expand Down
17 changes: 14 additions & 3 deletions packages/core/src/model/nodes/HttpCallNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ export class HttpCallNodeImpl extends NodeImpl<HttpCallNode> {
id: 'json' as PortId,
title: 'JSON',
},
{
dataType: 'binary',
id: 'binary' as PortId,
title: 'Binary',
},
{
dataType: 'number',
id: 'statusCode' as PortId,
Expand Down Expand Up @@ -244,15 +249,16 @@ export class HttpCallNodeImpl extends NodeImpl<HttpCallNode> {
},
};

const responseBody = await response.text();
const responseBlob = await response.blob();
const responseText = await responseBlob.text();

output['res_body' as PortId] = {
type: 'string',
value: responseBody,
value: responseText,
};

if (response.headers.get('content-type')?.includes('application/json')) {
const jsonData = JSON.parse(responseBody);
const jsonData = JSON.parse(responseText);
output['json' as PortId] = {
type: 'object',
value: jsonData,
Expand All @@ -264,6 +270,11 @@ export class HttpCallNodeImpl extends NodeImpl<HttpCallNode> {
};
}

output['binary' as PortId] = {
type: 'binary',
value: new Uint8Array(await responseBlob.arrayBuffer()),
};

return output;
} catch (err) {
const { message } = getError(err);
Expand Down