-
Notifications
You must be signed in to change notification settings - Fork 117
Add vs-constr-render to ValueSkeleton for Custom Rendering #1805
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
base: horizon
Are you sure you want to change the base?
Conversation
I noticed this PR a few days ago, but I don't currently understand it (though I'm sure we discussed this design at some point, I just don't recall it right now) - is there an example of using this new construction? |
Thanks for checking in! There's an example of how this might hook up to CPO here: brownplt/code.pyret.org#597 I don't have a CLI-specific example, but in general the idea is to be able to say "I'm going to take control of how this value is rendered".
|
function renderValueSkeleton(val, values) { | ||
if (thisRuntime.ffi.isVSValue(val)) { return values.pop(); } // double-check order! | ||
else if (thisRuntime.ffi.isVSStr(val)) { return thisRuntime.unwrap(thisRuntime.getField(val, "s")); } | ||
else if (thisRuntime.ffi.isVSConstrRender(val) && isInRendererContext(val)) { | ||
var items = thisRuntime.ffi.toArray(thisRuntime.getField(val, "args")); | ||
var strs = []; |
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.
These aren't actually guaranteed to be strings, though, right? (It could be arbitrary HTML or something?)
strs.push(renderValueSkeleton(items[i], values)); | ||
} | ||
const renderers = thisRuntime.getField(val, "renderers"); | ||
return thisRuntime.getField(renderers, thisContext).app(strs); |
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.
What happens when this app
returns a value that's not of the expected type? E.g. returns a DOM node when it's supposed to be a raw string? Or if it crashes/throws an error?
In general, I'm concerned about allowing arbitrary Pyret code to run during rendering, such that if the Pyret code is buggy/unexpected, the overall rendering algorithm will choke and produce no output at all, or the dreaded "error while rendering; details logged to console" message... Do you have a fallback here for what to do if that .app
fails for whatever reason?
@@ -202,9 +202,24 @@ function (Namespace, jsnums, codePoint, util, exnStackParser, loader, seedrandom | |||
*/ | |||
function isBase(obj) { return obj instanceof PBase; } | |||
|
|||
const thisContext = "cli"; |
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.
This feels very brittle to me. I also don't see where this gets changed for other contexts -- you have it marked as a const
, so line 221 will never look for anything other than cli
context, right? (In the PR you linked to, it's a const
of cpo
, which again is rigid.
Also -- how does this interact with to-repr
and to-string
?
This PR introduces a new variant to the ValueSkeleton data type: vs-constr-render.
vs-constr-render enables more flexible rendering of value skeletons by allowing the attachment of renderer functions to constructor-like skeletons.