refactor: app-data-evaluator

[chrismclarke]

PR Checklist

• PR title descriptive (can be used in release notes)

Description

Work on PRs #2388 #2454 is currently blocked due to challenges trying to get item rows to evaluate correctly from items, data_items and external actions contexts.

At the core of the issue is the way in which a general template parser does a 1-time pass of all data before it is sent to the relevant items/data_items processors, replacing dynamic variables where known and leaving partial expressions as-is.

This becomes problematic when using items that need to evaluate some sort of @item logic, as associated variables could be outdated and/or replaced with values that no longer evaluator. E.g. @item.name=== @local.name will reach the expression as @item.name===name_without_quotation. Even if defining @item.name==="@local.name" we can run into issues with quoted "undefined" values and such. There's also additional issues created when working with item actions that target different items (e.g. set_item: _index:1).

All of these issues are also extracted away behind multiple layers of templateParser logic which are incredibly confusing and hard to follow (lots of hack/temp workarounds to fix legacy bugs)

Moving forwards it seems like the best solution would be to completely detach the data_items processor from the main parser, and let it handle it's own evaluation methods. This also would offer a nice staging ground/proof-of-concept for implementing an alternative template parser (or at least offer ways to start decoupling some of the more complex parts)

This PR is a step towards decoupling, by refactoring the existing AppStringParser to better handle mixed use cases of JS and string evaluation. This parser will then be used as a drop-in replacement to process dynamic templates within data_items

This should unblock the immediate issues with #2454

Dev Notes

Unfortunately I made the age-old mistake of both renaming a file and making changes, so the diffs appear larger than in reality. The main change is to change the order of priority of the previous AppSringEvaluator so that it first attempts to evaluate as JS before using a fallback of string replacement. This should allow for any valid javascript expression to be evaluated.

The updated tests should capture the main use-cases, including the previously broken and disabled test:

it("@row.first_name === 'Ada'", () => {
    pending("TODO - fix implementation to support");
    expect(evaluator.evaluate("@row.first_name === 'Ada'")).toEqual(true);
  });

Git Issues

Closes #

Screenshots/Videos

If useful, provide screenshot or capture to highlight main changes

[jfmcquade]

Looks good, it was helpful to discuss on the call last week

[jfmcquade]

I'm not sure if trying to evaluate as js and catching the error case is hacky or not... Either way, feels like a clever way to do handle this and good to have it explicated

[chrismclarke]

It's incredibly hacky, but we're working with the restriction that authors expect to be able to interchangeably write both valid javascript @local.number > 3 and templated text hello @local.name. There was a time when we expected authors to wrap calculations in @calc(....) syntax, but that isn't used very frequently in some of the more modern examples.

The other option would be to try and analyze in advance what type of expressions are being used, e.g. have scripts in the parser that look for any unquoted strings, however this also gets quite fiddly given that some text may still be valid javascript (e.g. this., Number(...), true. Long term this is likely better, but would require a lot more work to specify how best to markup (guessing maybe better to track in metadata rather than replacing @calc statements everywhere), and authoring ways to enforce a particular pattern in case not evaluated correctly (e.g. escaping or forcing non-calc)

[jfmcquade]

As discussed on call last week, the fact that the evaluator is only used here should mean there's little potential for unexpected knock-ons with this PR