migrate to using vastly in mavoscript #1003
Conversation
✅ Deploy Preview for getmavo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -628,7 +604,7 @@ var _ = Mavo.Script = { | |||
| return `${nameSerialized}(${argsSerialized.join(", ")})`; | |||
| }, | |||
| "ConditionalExpression": node => `${_.serialize(node.test, node)}? ${_.serialize(node.consequent, node)} : ${_.serialize(node.alternate, node)}`, | |||
| "MemberExpression": (node, parent) => { | |||
There was a problem hiding this comment.
I deleted this because it was an unused variable
|
|
||
| if (typeof ret == "object" && ret?.type) { | ||
| node = ret; | ||
| Vastly.parents.set(node, parent); |
There was a problem hiding this comment.
I originally wanted to write this function as:
serialize: (node) => {
Vastly.parents.setAll(node);
return Vastly.serialize(node);
},However, due to serializers needing to pass a parent argument back into serialize, we have to do
serialize: (node, parent) => {
if (parent) {
Vastly.parents.set(node, parent);
}
return Vastly.serialize(node);
},
There was a problem hiding this comment.
But is that parent argument used? It seems to be primarily used to set the parent, which we already do.
There was a problem hiding this comment.
That's what I thought too, but it fails the majority of the serialize tests when implented like that, I'm still working on trying to figure out why that's the case
There was a problem hiding this comment.
I will update once I figure it out
There was a problem hiding this comment.
Hm, so it seems all related to it rewriting Mavo functions to use get() as well, so e.g. instead of $fn.eq() you get $fn.get($fn, "eq"), which happens here. So you need to verify that what you're doing is actually setting parents, cause them not being set properly would be my top guess (or maybe they're not properly set after transformations).
There was a problem hiding this comment.
@LeaVerou Coming back to this bug, it seems the root cause is that serializers mutate the node, meaning the parents originally set by vastly are no longer correct, an example is here.
I think having
if (parent) {
Vastly.parents.set(node, parent);
}isn't the worst solution, but I'll try to think of a way around it
There was a problem hiding this comment.
Note that what a lot of this logic does is trying to find the topmost variable, which you already have a function for!
Then, it's rewriting to prepend with $fn, which seems useful enough in itself that might be worth including in vastly (in a more generic form). E.g. we'll definitely need this for rewriting data variables to branch off $data as well, and for rewriting where calls (to branch off a function call with the possible alternatives). If this is handled by vastly, parents can be adjusted by vastly and calling code doesn't have to care about maintaining parent pointers.
There was a problem hiding this comment.
I'll create an issue for it over in vastly
There was a problem hiding this comment.
|
Nit: The filename should be |
|
Going to come back to this now |
No description provided.