-
Notifications
You must be signed in to change notification settings - Fork 6
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 support for object returns and nested JSX elements #28
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,21 +1,22 @@ | ||||||||||||||
import { JSX, MaybePromise } from "./jsx-runtime"; | ||||||||||||||
import { JSX } from "./jsx-runtime"; | ||||||||||||||
import { ComponentProps, MaybePromise, WorkflowComponent } from "./types"; | ||||||||||||||
|
||||||||||||||
export function Component<TInput, TOutput>( | ||||||||||||||
fn: (input: TInput) => MaybePromise<TOutput> | JSX.Element, | ||||||||||||||
) { | ||||||||||||||
function WorkflowFunction( | ||||||||||||||
props: TInput & { | ||||||||||||||
children?: ( | ||||||||||||||
output: TOutput, | ||||||||||||||
) => MaybePromise<TOutput | JSX.Element | JSX.Element[]>; | ||||||||||||||
}, | ||||||||||||||
): Promise<TOutput> { | ||||||||||||||
return Promise.resolve(fn(props)) as Promise<TOutput>; | ||||||||||||||
export function Component<P, O>( | ||||||||||||||
fn: (props: P) => MaybePromise<O | JSX.Element | JSX.Element[]>, | ||||||||||||||
): WorkflowComponent<P, O> { | ||||||||||||||
function WorkflowFunction(props: ComponentProps<P, O>): MaybePromise<O> { | ||||||||||||||
return Promise.resolve(fn(props)) as Promise<O>; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
if (fn.name) { | ||||||||||||||
Object.defineProperty(WorkflowFunction, "name", { | ||||||||||||||
value: `WorkflowFunction[${fn.name}]`, | ||||||||||||||
}); | ||||||||||||||
} | ||||||||||||||
return WorkflowFunction; | ||||||||||||||
|
||||||||||||||
// Mark as workflow component and JSX element type | ||||||||||||||
const component = WorkflowFunction as WorkflowComponent<P, O>; | ||||||||||||||
component.isWorkflowComponent = true; | ||||||||||||||
|
||||||||||||||
Comment on lines
+17
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, let's make it clear this is an internal thing:
Suggested change
|
||||||||||||||
return component; | ||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,67 @@ | |
*/ | ||
|
||
import { Component } from "./component"; | ||
import { execute } from "./gensx"; | ||
import { execute } from "./resolve"; | ||
import { Element, ExecutableValue } from "./types"; | ||
|
||
// Collect component props | ||
export interface CollectProps { | ||
children: Element[]; | ||
} | ||
|
||
// Export everything through gsx namespace | ||
export const gsx = { | ||
Component, | ||
execute, | ||
Collect, | ||
}; | ||
|
||
// Export Component and execute directly for use in type definitions | ||
export { Component, execute }; | ||
|
||
// Also export types | ||
export type { Element }; | ||
|
||
// Collect component for parallel execution with named outputs | ||
export async function Collect<T extends Record<string, unknown>>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move this to its own file? Let's try to keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this code gets deleted in a later PR. It is no longer needed. |
||
props: CollectProps, | ||
): Promise<T> { | ||
const children = Array.isArray(props.children) | ||
? props.children | ||
: [props.children]; | ||
|
||
// Execute all children in parallel | ||
const results = await Promise.all( | ||
children.map(child => { | ||
/* eslint-disable @typescript-eslint/no-unsafe-assignment */ | ||
/* eslint-disable @typescript-eslint/no-explicit-any */ | ||
/* eslint-disable @typescript-eslint/no-unsafe-member-access */ | ||
/* eslint-disable @typescript-eslint/no-unsafe-argument */ | ||
Comment on lines
+48
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can move these to the top of the ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call. |
||
const outputName = (child as any)?.props?.output; | ||
return executeWithName(child, outputName); | ||
/* eslint-enable @typescript-eslint/no-unsafe-assignment */ | ||
/* eslint-enable @typescript-eslint/no-explicit-any */ | ||
/* eslint-enable @typescript-eslint/no-unsafe-member-access */ | ||
/* eslint-enable @typescript-eslint/no-unsafe-argument */ | ||
}), | ||
); | ||
|
||
// Combine results into an object | ||
const output = {} as Record<string, unknown> as T; | ||
for (const [name, value] of results) { | ||
if (name) { | ||
(output as Record<string, unknown>)[name] = value; | ||
} | ||
} | ||
|
||
return output; | ||
} | ||
|
||
// Execute with output name tracking | ||
async function executeWithName<T>( | ||
element: Element, | ||
outputName?: string, | ||
): Promise<[string | undefined, T]> { | ||
const result = await execute<T>(element as ExecutableValue); | ||
return [outputName, result]; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
/* eslint-disable @typescript-eslint/no-namespace */ | ||
import { resolveDeep } from "./resolve"; | ||
|
||
export namespace JSX { | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
export type ElementType = (props: any) => Promise<unknown>; | ||
|
@@ -18,7 +20,6 @@ export const Fragment = (props: { | |
if (Array.isArray(props.children)) { | ||
return props.children; | ||
} | ||
|
||
return [props.children]; | ||
}; | ||
|
||
|
@@ -31,30 +32,39 @@ export const jsx = < | |
component: (props: TProps) => MaybePromise<TOutput>, | ||
props: TProps | null, | ||
children?: (output: TOutput) => MaybePromise<JSX.Element | JSX.Element[]>, | ||
): Promise<TOutput | TOutput[]> => { | ||
): Promise<Awaited<TOutput> | Awaited<TOutput>[]> => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TOutput is not a PromiseLike, we don't want Awaited here I don't think (even if the compiler really thinks we do). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But the component could return a direct value or a promise, no? Something I had to solve to get this to work is traversing every element since it can contain:
And then making sure that every promise is awaited, and that every element is evaluated recursively until every returned value is plain. Though very possible I am missing something here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But the component returns |
||
if (!children && props?.children) { | ||
children = props.children; | ||
} | ||
return Promise.resolve(component(props ?? ({} as TProps))).then(result => { | ||
if (children) { | ||
// If its an array of elements, this is an edge case for a Fragment. | ||
if (Array.isArray(children)) { | ||
return Promise.all(children); | ||
} | ||
if (typeof children === "function") { | ||
// If the components child function returns an array of elements, we need to resolve them all | ||
const childrenResult = children(result); | ||
if (Array.isArray(childrenResult)) { | ||
return Promise.all(childrenResult); | ||
} | ||
return Promise.resolve(childrenResult); | ||
} | ||
|
||
// If its a single element, this is an edge case for a Fragment. | ||
return Promise.resolve(children); | ||
// Return a promise that will be handled by execute() | ||
return (async (): Promise<Awaited<TOutput> | Awaited<TOutput>[]> => { | ||
// Execute component and deeply resolve its result | ||
const rawResult = await component(props ?? ({} as TProps)); | ||
const result = (await resolveDeep(rawResult)) as TOutput; | ||
|
||
// If there are no children, return the resolved result | ||
if (!children) { | ||
return result as Awaited<TOutput>; | ||
} | ||
return result; | ||
}) as Promise<TOutput | TOutput[]>; | ||
|
||
// Handle array of children (Fragment edge case) | ||
if (Array.isArray(children)) { | ||
const resolvedChildren = await Promise.all( | ||
children.map(child => resolveDeep(child)), | ||
); | ||
return resolvedChildren as Awaited<TOutput>[]; | ||
} | ||
|
||
// Handle child function | ||
if (typeof children === "function") { | ||
const childrenResult = await children(result); | ||
return resolveDeep(childrenResult) as Awaited<TOutput>; | ||
} | ||
|
||
// Handle single child (Fragment edge case) | ||
return resolveDeep(children) as Awaited<TOutput>; | ||
})(); | ||
}; | ||
|
||
export const jsxs = < | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
import { JSX } from "./jsx-runtime"; | ||
import { ComponentProps, ExecutableValue, WorkflowComponent } from "./types"; | ||
|
||
// Helper to check if something is a JSX element | ||
/* eslint-disable @typescript-eslint/no-unsafe-return */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might as well just move these to the top of the file I think. |
||
/* eslint-disable @typescript-eslint/no-explicit-any */ | ||
/* eslint-disable @typescript-eslint/no-unsafe-member-access */ | ||
function isJSXElement<P, O>( | ||
element: unknown, | ||
): element is JSX.Element & { | ||
type: WorkflowComponent<P, O>; | ||
props: ComponentProps<P, O>; | ||
} { | ||
return ( | ||
typeof element === "object" && | ||
element !== null && | ||
"type" in element && | ||
"props" in element && | ||
typeof (element as any).type === "function" && | ||
Comment on lines
+17
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't use the |
||
(element as any).type.isWorkflowComponent | ||
); | ||
} | ||
/* eslint-enable @typescript-eslint/no-unsafe-return */ | ||
/* eslint-enable @typescript-eslint/no-explicit-any */ | ||
/* eslint-enable @typescript-eslint/no-unsafe-member-access */ | ||
|
||
/** | ||
* Deeply resolves any value, handling promises, arrays, objects, and JSX elements. | ||
* This is the core resolution logic used by both execute() and the JSX runtime. | ||
*/ | ||
export async function resolveDeep<T>(value: unknown): Promise<T> { | ||
// Handle promises first | ||
if (value instanceof Promise) { | ||
/* eslint-disable @typescript-eslint/no-unsafe-assignment */ | ||
const resolved = await value; | ||
return resolveDeep(resolved); | ||
/* eslint-enable @typescript-eslint/no-unsafe-assignment */ | ||
} | ||
|
||
// Handle arrays | ||
if (Array.isArray(value)) { | ||
const resolvedArray = await Promise.all( | ||
value.map(item => resolveDeep(item)), | ||
); | ||
return resolvedArray as T; | ||
} | ||
|
||
// Handle JSX elements | ||
if (isJSXElement(value)) { | ||
const componentResult = await value.type(value.props); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't figure out where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And I'm not sure we need to follow the same pattern, I've avoided the extra complexity so far. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I thought it was being assigned at the framework layer, but it looks like that must've been lost at some intermediate step. I'm surprised that it works at all with type being undefined, but it looks like isJSXElement always returns false, but resolveDeep ends up doing the right thing. We should talk through the right refactor here. Maybe we can just remove most of this code. |
||
return resolveDeep(componentResult); | ||
} | ||
|
||
// Handle objects (but not null) | ||
if (typeof value === "object" && value !== null) { | ||
const entries = Object.entries(value); | ||
const resolvedEntries = await Promise.all( | ||
entries.map(async ([key, val]) => [key, await resolveDeep(val)]), | ||
); | ||
return Object.fromEntries(resolvedEntries) as T; | ||
} | ||
|
||
// Base case: primitive value | ||
return value as T; | ||
} | ||
|
||
/** | ||
* Executes a JSX element or any other value, ensuring all promises and nested values are resolved. | ||
* This is the main entry point for executing workflow components. | ||
*/ | ||
export async function execute<T>(element: ExecutableValue): Promise<T> { | ||
/* eslint-disable @typescript-eslint/no-unnecessary-condition */ | ||
if (element === null || element === undefined) { | ||
throw new Error("Cannot execute null or undefined element"); | ||
} | ||
/* eslint-enable @typescript-eslint/no-unnecessary-condition */ | ||
|
||
// Handle JSX elements specially to support children functions | ||
if (isJSXElement(element)) { | ||
const componentResult = await element.type(element.props); | ||
const resolvedResult = await resolveDeep(componentResult); | ||
|
||
// Handle children after fully resolving the component's result | ||
if (element.props.children) { | ||
const childrenResult = await element.props.children(resolvedResult); | ||
return execute(childrenResult as ExecutableValue); | ||
} | ||
|
||
return resolvedResult as T; | ||
} | ||
|
||
// For all other cases, use the shared resolver | ||
return resolveDeep(element); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally unrelated to this change, but I wonder if we should change the name of this to
GsxComponent
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can if we have a good reason.
We should work backwards from what we want the examples code to look like.
gsx.Component
feels good IMO, users can alwaysimport { Component as GsxComponent ..
if they want, but I'm not married to the current convention either.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry, I meant specifically this string (and the name of the
WorkflowFunction
wrapper). So if the user doesconsole.log
they get something specific to gsx and not generic likeWorkflowFunction[MyFunction]