Skip to content
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

Migrating away from with() so we can move to ESM #967

Open
2 of 4 tasks
LeaVerou opened this issue Sep 2, 2023 · 16 comments
Open
2 of 4 tasks

Migrating away from with() so we can move to ESM #967

LeaVerou opened this issue Sep 2, 2023 · 16 comments

Comments

@LeaVerou
Copy link
Member

LeaVerou commented Sep 2, 2023

The primary reason Mavo is not ESM yet is that we use with() to evaluate expressions:

ESM enforces strict mode, which does not allow with().

Refactoring our code so that it doesn't use with() will not only allow us to move to ESM, but will also make expression evaluation faster (though it's hard to tell how much).

This is not a simple undertaking, but I do think it's possible. We're already rewriting function calls to be called on $fn (in the past we used with() for that too), we need to do something similar for data as well. Something like this:

  1. Find all operands and rewrite them to start from $data (e.g. foo + bar.baz would become $data.foo + $data.bar.baz)
  2. Define $data right under $fnto something like:let $data = data ?? Mavo.Data.stub;
  3. Done!

Number 1 is the tricky bit. I've had a lot of false starts trying to write an algorithm to correctly extract all operands from an expression. It seems deceptively simple, but is not if you take all the corner cases into account. Perhaps we need a structured effort:

  1. Collect many expression examples from a variety of Mavo apps. It should be relatively easy to write a bookmarklet that traverses all Expression objects and prints an array in the console. Or even one that appends them to a JSON file, so all we need to do is login (if we're not already logged in), press the button and chill.
  2. Pare down these expression examples to those that showcase different AST structures that we need to recognize (e.g. no point in having foo + bar and a * b, these have the same structure, they're both BinaryExpression nodes with a left and right that are Identifier nodes)
  3. Convert these into tests
  4. At this point it should be easier to come up with a good algorithm and verify that it works well.

Rewriting operands to be chained on $data will also make it easier to maybe one day rewrite Mavo to use another reactivity engine under the hood, which will resolve the source of a great chunk of bugs.

PS: We'd need to release a new stable version before any of this work. We need to fix 3 regressions for that to happen, none of which seem particularly hard.

Tasks

@karger
Copy link
Collaborator

karger commented Sep 3, 2023

"Find all operands" seems a necessary step for all PL compilers (parsers). Can you steal someone else's algorithm? Is there something that makes it unusually difficult for Mavo?

@LeaVerou
Copy link
Member Author

LeaVerou commented Sep 3, 2023

"Find all operands" seems a necessary step for all PL compilers (parsers). Can you steal someone else's algorithm? Is there something that makes it unusually difficult for Mavo?

Indeed; and the AST format is standardized. I wondered about this too, someone (either @DmitrySharabin or a UROP/MEng) just needs to do the research

@DmitrySharabin
Copy link
Member

I have found a couple of articles that might be helpful, including the one from Douglas Crockford—the creator of JSLint:

  1. Pratt Parsing and Precedence Climbing Are the Same Algorithm
  2. Top Down Operator Precedence

@LeaVerou
Copy link
Member Author

LeaVerou commented Oct 1, 2023

From a quick skim, not sure these help in extracting operands? Can you point me to the relevant place?

@LeaVerou
Copy link
Member Author

LeaVerou commented Oct 3, 2023

I created a couple issues with detailed steps towards getting to this (having @adamjanicki2 in mind) and added them to a tasklist at the end of the first post. Eventually we'd need to move to a project, but that's probably fine for now.

@adamjanicki2 may end up finding an existing algorithm for this instead, but if not, these steps are almost guaranteed to get us there. We should probably eventually release these helpers as a separate library for working with expression ASTs, since there's practically nothing Mavo-specific there.

@adamjanicki2
Copy link
Collaborator

Attempt 1 for taking an expression, finding all variables, filtering variables and functions, and rewriting them all to have $data or $fn prefixes; this is just a general, first pass

import * as parents from "./parents.js";
import variables from "./variables.js";
import serialize from "./serialize.js";
import jsep from "jsep";

function addMemberPrefix(node, prefix) {
    if (node.type === "Identifier") {
        node.type = "MemberExpression";
        node.computed = false;
        node.object = {type: "Identifier", name: prefix};
        node.property = {type: "Identifier", name: node.name};
        delete node.name;
    }
}

const expr = "a.b + func(a, b)";
const ast = jsep(expr);
parents.setAll(ast);
const identifiers = variables(ast);

const vars = identifiers.filter(node => node !== node.parent?.callee);
const functions = identifiers.filter(node => node === node.parent?.callee);

for (const variable of vars) {
    addMemberPrefix(variable, "$data");
}

for (const func of functions) {
    addMemberPrefix(func, "$fn");
}

console.log(serialize(ast)); // logs: $data.a.b + $fn.func($data.a, $data.b)

@adamjanicki2
Copy link
Collaborator

@LeaVerou thoughts on this?

@LeaVerou
Copy link
Member Author

LeaVerou commented Nov 21, 2023

@adamjanicki2 Sorry, I assumed you were still iterating. Just checking node.parent.callee does not seem like it would work well for any but the most trivial of expressions, but I could be wrong and it may work in practice. Have you tested it with the dataset of expressions you've collected?

As a simple example off the top of my head, you could have name.includes(selectedName) where name and selectedName are variables and need to be prefixed with $data, but name.includes is in the callee.
In fact, currently none of Mavo functions are namespaced, so when we have a MemberExpression in the callee, it's either a function call to the host environment (no $fn prefixing needed, but we should keep track of the topmost identifiers to expose it as a global if we move to stricter sandboxing, e.g. like the one Vue imposes), or a method on a property (which needs a $data prefix).

@adamjanicki2
Copy link
Collaborator

In fact, currently none of Mavo functions are namespaced, so when we have a MemberExpression in the callee, it's either a function call to the host environment (no $fn prefixing needed, but we should keep track of the topmost identifiers to expose it as a global if we move to stricter sandboxing, e.g. like the one Vue imposes)

Can you explain this a bit more? What does function call to the host environment mean?

or a method on a property (which needs a $data prefix).

So this case is properly handled by the code I pasted above, for example, the example you provided, name.includes(selectedName) would get rewritten to $data.name.includes($data.selectedName) (this is the intended behavior, right?)

@LeaVerou
Copy link
Member Author

In fact, currently none of Mavo functions are namespaced, so when we have a MemberExpression in the callee, it's either a function call to the host environment (no $fn prefixing needed, but we should keep track of the topmost identifiers to expose it as a global if we move to stricter sandboxing, e.g. like the one Vue imposes)

Can you explain this a bit more? What does function call to the host environment mean?

E.g. functions like Array.isArray(), navigator.share(), document.write() and so on. I.e. something that is interpreted by the host environment with almost no inteverention by Mavo.
So foo.bar() could either mean that foo is a global, or that foo is a property, and bar() a method on it.

@adamjanicki2
Copy link
Collaborator

adamjanicki2 commented Nov 22, 2023

Ok makes sense, I see what you mean. So instead of the naive approach I have above, what if we check for membership in globalThis, and then append it to a special modifier like $global? Something like this:

...
for (const variable of vars) {
    const prefix = globalThis[variable.name] ? "$global" : "$data";
    addMemberPrefix(variable, prefix);
}

for (const func of functions) {
    const prefix = globalThis[func.name] ? "$global" : "$fn";
    addMemberPrefix(func, prefix);
}
...

Then an expression like Array.isArray(arr) would become $global.Array.isArray($data.arr). Would this make it easier to distinguish between items that need to be evaluated in host environment vs locally by Mavo? Do you have any additional thoughts?

@LeaVerou
Copy link
Member Author

Yes, we could do this for now.

@adamjanicki2
Copy link
Collaborator

Would having a function for "concatenating" member expressions (like above) be useful to include in vastly, or should this be implemented in Mavo?

@LeaVerou
Copy link
Member Author

Would having a function for "concatenating" member expressions (like above) be useful to include in vastly, or should this be implemented in Mavo?

Yes, I think this could absolutely be more broadly useful! The only downside of handling this in vastly is that iit may need to handle more use cases than what Mavo needs.

@adamjanicki2
Copy link
Collaborator

How about we implement this in mavo for now so we can move forward with removing with, and we can come back to this and move it to vastly once we understand the general use cases more?

@LeaVerou
Copy link
Member Author

How about we implement this in mavo for now so we can move forward with removing with, and we can come back to this and move it to vastly once we understand the general use cases more?

Sounds great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants