spike(action_lists)!: evaluation standalone

[chrismclarke]

PR Checklist

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

Breaking Changes

action_list evaluates using newer templated data system
This may lead to some unexpected behaviour if any action_lists include complex statements that mix variable references, e.g. click | set_field : some_field : @local.some_value + 1. I think these types of patterns had always been discouraged/not worked anyways (with preference of setting intermediate local variables instead), but just to be aware of some small potential for knock-on. Unfortunately we don't have many tests for action_list edge cases so might be good to check against existing debug sheets.

action_list values references now auto-update
Previous workaround to specify this.value in action_lists to handle receiving updated value for triggered actions are no longer required. The syntax is still supported and not marked as deprecated (no plans to remove), but any documentation that refers to this could likely be updated.

Description

Action lists are currently evaluated as part of main template container processing, meaning that all row action params and args are evaluated prior to the row that triggers them from rendering. There are 2 main drawbacks to this:

1. Actions need to be parsed even if they are never actually triggered. This isn't too expensive an operation, just lacking a bit in optimisation.

2. Actions can become stale if data changes between render and trigger. This typically happens if an is triggered by a component change (parsed action will have value before change), or if multiple actions are triggered in series where the first action manipulates data required for the second action.

This PR aims to detach action_list processing from the main templating system and instead only evaluate at the time of being triggered. This should address both issues above

Additionally it evaluates actions using the new app data templating system, which has better support for complex expressions such as inline JS evaluation

TODO

• Add Tests (likely following merge of Fix: ng tests #2742)
• Review handling of standalone actions to make sure work as expected (think they are already parsed independently, but should check)
• Ensure fix: action parameter self-references #2749 and hotfix: action parameter self-reference concatenation #2750 debug cases still work
• Review previous interceptTemplateContainerAction from layout nav-group and refactor interceptor as required

Author Notes

(TODO)

Review Notes

(TODO - identify which sheets best to check/debug functionality, create new sheets as required)

Dev Notes

The main changes here are to omit action_list fields from the template-row service evaluator, and add a new interceptor to the template container that sets values of any dynamic variables at the point of being triggered.

There was a choice between trying to use the existing row processing system to just delay evaluation of actions until the point of being triggered, or migrate to the new app data templating system that was implemented to evaluate dynamic variables within data-items. I opted for the latter as long term I want to fully remove the legacy system as it focuses too much on individual string replacement and not evaluating expressions.

This means in the short term additional methods added to the template-variables to allow specific prefix evaluation (e.g. global, field, local) for either source of data - legacy TemplateRowDynamicEvaluator types (includes partial lookup/matches of things like fulExpression and matchedExpression), as well as the newer ITemplatedDataContextList type which is essentially a dummy-table containing a list of all variables used so that they can be evaluated within a JS context (instead of string replacement). It also means refactoring some methods so that the can be exposed publically for use within the interceptor that evaluates action_list variables standalone

Additionally, because the system of interceptors recently introduced only supports 1 interceptor per scope, where a scope is typically the nested name of a component that wants to intercept child actions (e.g. so a data_items container can intercept all item children actions), a small workaround has been added to allow a single non-scoped interceptor that applies to all actions within a container (each container creates it's own template-action service so can still have multiple across containers).

Follow-up

Beyond scope of this PR, but could be considered for future follow-up

• Consider updating parser to extract context variables dummy tables within parser instead of at runtime. May also want to consider replacing dummy table marker fields: {some_field: true} to object that can store metadata (if useful), e.g.

fields:{some_field:{value:'',type:'string'}}

• Add support for dynamic evaluator registry so that specific services can register how to evaluate context variable prefixes (e.g. @field, @global)
• Consider deprecating this. workarounds (or maybe add @this/@self namespace?)
• Remove need to store _triggeredBy with actions (template container should be able to manage)
• Consider migrating all actions to use params instead of args for more consistency (or better way to define args)
• Include service init await in template-variables.service so that they do not need to be explicitly included in places like template-process.service or app.component.ts

Git Issues

Closes #

Screenshots/Videos

If useful, provide screenshot or capture to highlight main changes

[chrismclarke]

@jfmcquade
I've still got a bit more work to do on this PR but I was wondering if I could ask you to take a first pass for initial impressions and whether you think anything could do with some additional explanation/tidying/refactor?

[jfmcquade]

Thanks @chrismclarke. I've only had a chance for a quick look through, but the changes definitely make sense from a high level, it seems right that actions should evaluate only at the point of being triggered. I should get a chance to look in a bit more detail tomorrow.

In terms of timelines, because of the potential for knock-ons with action handling and the critical status of two deployments currently being worked on (plh_kids_kw and plh_facilitator_mx), I think it probably makes sense to wait until those have been released before merging these changes. Although saying that, it would be good to discuss with @esmeetewinkel and @ChrisMarsh82 if they expect any knock-ons from the breaking changes you've highlighted. I've added this PR as a discussion point for our meeting tomorrow, we can decide how much depth we need to go into as a group.

[jfmcquade]

NB, this should resolve #2622