delete with and move to use vastly #1001
Conversation
✅ Deploy Preview for getmavo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| "object", "property", // MemberExpression | ||
| "body" | ||
| ], | ||
| walk: Vastly.walk, |
There was a problem hiding this comment.
@DmitrySharabin what do you think? Should we continue to define these or just use vastly methods internally and print out a deprecation warning for these, directing plugin authors to use vastly too (using your Mavo.Functions.deprecatedFunction())?
Might be helpful to quickly look into whether these are used by any plugins
There was a problem hiding this comment.
I checked all Mavo plugins. Not only ours but also every plugin in the plugin registry. I couldn't find any that uses these Mavo.Script methods: walk, closest, and serialize. However, even though we don't use it in the plugins from the registry, we can't guarantee no (private) plugin does. So, I would follow the route with a deprecation message (until the next release minimum). At least we can lower the chance of breaking some author code and level up the awareness of vASTly, which is nice from my perspective. 😊
| } | ||
|
|
||
| return _.serializers[node.type](node, parent); | ||
| }, |
There was a problem hiding this comment.
Oooh that must have been pleasant 😁
However, we still need the transformations (vastly has an export of an empty transformations object so the developer can modify them). Keep in mind vastly is written for parsing/serializing regular JS, so all the special Mavo stuff will still live here.
There was a problem hiding this comment.
Yeah I realized this after I committed it, oops! One question I had was how do we set the transformations for vastly from mavo?
There was a problem hiding this comment.
Try Object.assign(Vastly.transformations, transformations) but do check where rollup puts that export, it may be Vastly.serialize.transformations or something entirely different.
There was a problem hiding this comment.
Followup question: where should this go such that it only runs once? Somewhere around here?
There was a problem hiding this comment.
Update: transformations and other non-default exports are not accessible outside Vastly. I assume it's because the index file only exports the defaults for each file: https://github.com/mavoweb/vastly/blob/main/src/index.js#L2C1-L9C55
There was a problem hiding this comment.
Followup question: where should this go such that it only runs once? Somewhere around here?
I'm confused, Mavo's transformations wouldn't go in vastly.
There was a problem hiding this comment.
Yeah I agree, I was asking where in mavo should we put the line Object.assign(Vastly..., transformations). I suggested here since there's a very similar line right below to set something inside stretchy
There was a problem hiding this comment.
That's something to fix on the vastly side :) Please file an issue first, so you have something to close.
Will do
One way is to add it as a property of
serialize, another to add it as a separate named export. Which solution would you pick and why? :)
I'm leaning toward adding it as a prop of serialize, because adding additional lines like export {transformations} from "./serialize.js" doesn't seem too good to me. When you say add as a property of serialize, how would I go about doing this?
There was a problem hiding this comment.
Ok, let me elaborate a bit, as I think my earlier message wasn't super clear.
Option 1 is that index-fn also does export { transformations, serializers }. Then they become top-level exports, same as the functions, and they'd be available as e.g. Vastly.transformations.
Option 2 is that we add them as properties of the serialize function, since functions are objects in JS:
serialize.transformations = transformations;
Then they are not exported per se, but can be accessed via serialize.
What do you think are the pros & cons of each of these approaches?
There was a problem hiding this comment.
That's cool! To me, it seems like the pros of option1 is we don't waste space inside of serialize "object", and the pros of option2 are we don't have to worry about exporting a separate thing from index, and transformations is tied to serialize since that's where it belongs
Overall I would say going for option 2 is better, but if you have a strong opinion either way, let me know
Work in progress, not ready for review yet
Addresses #967