-
Notifications
You must be signed in to change notification settings - Fork 74
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
Add ability to run lambdas as plugin mustache extensions #951
base: master
Are you sure you want to change the base?
Conversation
Nice! I have some ideas how to support other objects on the output of Mustache than plain strings/markdown parts, but this implementation is a good start. |
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.
I've played with that code a bit, see the comments.
I was also trying to extend your solution with ability to return whole React components. That one (9db0736)) is an failed attempt because renderSection assumes that view function will return a string that is just concatenated to the output (https://github.com/janl/mustache.js/blob/972fd2b27a036888acfcb60d6119317744fac7ee/mustache.js#L612), but close enough, I just need to get rid of these LambdaReference objects and emit Markdown directly in some wrapper.
// In case of lambdas, the subrenderer makes | ||
// this.view be a MustacheContext so make sure | ||
// we get the actual view | ||
let currentObject = this.view instanceof MustacheContext | ||
? this.view.view | ||
: this.view; |
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.
That's caused by incorrect overload of Mustache.Writer.render
, I didn't know that it's called recursively (https://github.com/janl/mustache.js/blob/972fd2b27a036888acfcb60d6119317744fac7ee/mustache.js#L593).
I think the best solution is to create root MustacheContext in renderValue instead of overloading render
function isFunction(obj: Object): boolean { | ||
return typeof obj === "function"; | ||
} | ||
|
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.
I think we can use lodash _.isFunction
for that, definition is the same 😄 https://github.com/lodash/lodash/blob/a67a085cc0612f5b83d78024e507427dab25ca2c/src/isFunction.ts#L28
Meanwhile I found that lodash is not really maintained anymore 🤔
const lambdas = fromPlugins("mustacheExtensions").reduce( | ||
(prev, curr) => { | ||
return { | ||
...prev, | ||
..._.mapValues(curr, (func: stringOpFunc) => lambda(func)) | ||
} | ||
}, | ||
{}); | ||
|
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.
Global scope is too early for call to fromPlugins
as the list of plugin extensions is loaded lazily during initial render (https://github.com/CERT-Polska/mwdb-core/blob/master/mwdb/web/src/index.jsx#L19)
I think it should be moved to the renderValue scope.
const markdown = mustacheWriter.render( | ||
template, | ||
{ | ||
...value, | ||
"value": | ||
{ | ||
...(value as any).value, | ||
...lambdas | ||
} | ||
} | ||
); |
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.
value
doesn't have to be a dictionary object e.g. List of entries
example doesn't render correctly. It's better to put lambdas in top-level view object.
const markdown = mustacheWriter.render( | |
template, | |
{ | |
...value, | |
"value": | |
{ | |
...(value as any).value, | |
...lambdas | |
} | |
} | |
); | |
const markdown = mustacheWriter.render( | |
template, | |
{ | |
...value, | |
...lambdas | |
} | |
); |
The best solution would be a dedicated collection in MustacheContext.
@@ -135,6 +146,9 @@ class MustacheContext extends Mustache.Context { | |||
if (!query) return undefined; | |||
return new SearchReference(query, currentObject); | |||
} | |||
if (isFunction(currentObject)) { | |||
currentObject = currentObject.call(this.view); |
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.
I see that object returned by lookup is already called by Mustache.js as a part of renderSection
. Do we need to resolve it at lookup level?
https://github.com/janl/mustache.js/blob/master/mustache.js#L609
Ok, I managed to apply some fixes and extend it even more so you can emit arbitrary objects. See that one: #955 😃 |
Hey!
This PR adds the ability to (partially) support a feature of the Mustache language called "lambdas". Lambdas are defined in the Mustache user manual as so:
So essentially, if the object I'm trying to render is:
then
{{#wrapped}}{{name}}{{/wrapped}}
will be rendered as<b>Willy</b>
.As can be seen in this case, this allows for sections to be treated sort of like functions. So what we can do is let the user define his own functions, and let them extend the vanilla Mustache used in MWDB.
With this PR, if a user defines in their plugin:
Then they can achieve:
Concerns
(input: string) => string
or at the very least return a string. However, this is not enforced and there's no checking for that.