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.

Sign up for GitHub

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 4 commits into from
May 8, 2024
Merged

feat: add binary output port on HttpCallNode #392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2a70869
Add 'binary' output port
liaujianjie Apr 27, 2024
18961b2
Pass blob into binary output port
liaujianjie Apr 27, 2024
0451f99
Fix crash at <RenderDataValue> for binary type
liaujianjie Apr 27, 2024
7f54555
Add option to toggle between binary and text/json outputs
liaujianjie May 2, 2024
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
View file
Open in desktop
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
81 changes: 54 additions & 27 deletions packages/core/src/model/nodes/HttpCallNode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export type HttpCallNodeData = {
body: string;
useBodyInput?: boolean;

isBinaryOutput?: boolean;

errorOnNon200?: boolean;
};

Expand Down Expand Up @@ -93,17 +95,29 @@ export class HttpCallNodeImpl extends NodeImpl<HttpCallNode> {
}

getOutputDefinitions(): NodeOutputDefinition[] {
return [
{
dataType: 'string',
id: 'res_body' as PortId,
title: 'Body',
},
{
dataType: 'object',
id: 'json' as PortId,
title: 'JSON',
},
const outputDefinitions: NodeOutputDefinition[] = [];
if (this.data.isBinaryOutput) {
outputDefinitions.push({
dataType: 'binary',
id: 'binary' as PortId,
title: 'Binary',
});
} else {
outputDefinitions.push(
{
dataType: 'string',
id: 'res_body' as PortId,
title: 'Body',
},
{
dataType: 'object',
id: 'json' as PortId,
title: 'JSON',
},
);
}

outputDefinitions.push(
{
dataType: 'number',
id: 'statusCode' as PortId,
Expand All @@ -114,7 +128,9 @@ export class HttpCallNodeImpl extends NodeImpl<HttpCallNode> {
id: 'res_headers' as PortId,
title: 'Headers',
},
];
);

return outputDefinitions;
}

getEditors(): EditorDefinition<HttpCallNode>[] {
Expand Down Expand Up @@ -151,6 +167,11 @@ export class HttpCallNodeImpl extends NodeImpl<HttpCallNode> {
useInputToggleDataKey: 'useBodyInput',
language: 'json',
},
{
type: 'toggle',
label: 'Whether response body is expected to be a binary',
dataKey: 'isBinaryOutput',
},
{
type: 'toggle',
label: 'Error on non-200 status code',
Expand Down Expand Up @@ -244,24 +265,30 @@ export class HttpCallNodeImpl extends NodeImpl<HttpCallNode> {
},
};

const responseBody = await response.text();

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

if (response.headers.get('content-type')?.includes('application/json')) {
const jsonData = JSON.parse(responseBody);
output['json' as PortId] = {
type: 'object',
value: jsonData,
if (this.data.isBinaryOutput) {
const responseBlob = await response.blob();
output['binary' as PortId] = {
type: 'binary',
value: new Uint8Array(await responseBlob.arrayBuffer()),
};
} else {
output['json' as PortId] = {
type: 'control-flow-excluded',
value: undefined,
const responseText = await response.text();
output['res_body' as PortId] = {
type: 'string',
value: responseText,
};
if (response.headers.get('content-type')?.includes('application/json')) {
const jsonData = JSON.parse(responseText);
output['json' as PortId] = {
type: 'object',
value: jsonData,
};
} else {
output['json' as PortId] = {
type: 'control-flow-excluded',
value: undefined,
};
}
}

return output;
Expand Down
Loading