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

Refactor/set data action #2454

Draft
wants to merge 65 commits into
base: master
Choose a base branch
from
Draft

Conversation

chrismclarke
Copy link
Member

@chrismclarke chrismclarke commented Oct 3, 2024

PR Checklist

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

TODO

  • Update following Feat: set data action standalone #2513 merge
  • Consider refactor to just let set_data action handle (less efficient to repeat list lookup but should be more maintainable code)
  • Update item handling to remove any references in template-variable service (let item processor handle change @item -> this.item). Remove hack code from set_data action used to workaround
  • Look to fully deprecate items in favour of data_items

Update 2024-11-09
Core set_data handling moved to standalone PR #2513 to try unblock and make easier to review

Update 2024-10-30
With further work on this feature it has become apparent that a complete rewrite is required, fully decoupling the data_items from both items and template processors.

Problem Statement
The fundamental issue is the way in which dynamic data is evaluated in stages, with a first pass at the templating level and then a second pass at the item level. This makes it impossible to evaluate data that combines both local and item contexts (e.g. @item.number > @local.number). The item code also contains a number of workarounds for partially parsed data which are hard to extend or work with, and is lacking in testing infrastructure to ensure changes do not have unforseen knock-ons.

The existing item implementation is also lacking some functionality, such as the ability to refer to a target item with the correct context, e.g. to toggle the completed state of the next item you may try to use an action like

set_item | _index: @item._index + 1; completed: [email protected]`

however both the _index lookup and completed lookup will operate on the current item. Similarly set_items | completed: [email protected] will set all item values to the initial item row value

Solution
The proposed solution is to completely rewrite the data_items system to be decoupled from both templating and items. It is intended that the rewrite is fully backwards-compatible with the current data_items syntax (no re-authoring required), and that once implemented it could also potentially be a replacement for items, unifying the two syntaxes.

The solution also provides an alternative pathway to managing content with dynamic variables, which in turn could form the basis of a new templating system (discussed in Templating RFC)


Blocking
The system works when managing @items references, however an initial pass on item processing is handled in the main processor which also replaces @item with this.item references. Whilst it is possible to just add a hackUnparseItem method, this feels messy. Instead it would be better to improve the item handling at the parser level, to avoid replacing outside of the item processor. The challenge here is that the initial items template processor is reasonably untested and quite fragile, so changing as part of this PR isn't advisable

Due to the closely coupled nature of the parser and items the best solution appears to be to mostly detach the data_items
parsing from the template parser, and essentially handle it's own parsing from the ground up. Whilst it is still possible to expose and call methods from the current parser (e.g. processDynamicEvaluators), these methods themselves are quite hacky and have a number of issues (particularly regarding the way dynamic variables are replaced as strings one-by-one, making it harder to evaluate compound expressions, e.g. @row.first_name === 'Ada' replaces first_name without quotation).

However a full solution for this will likely involve creating a new base parser that can convert full expressions to valid js and evaluate. E.g.
Hello @field.{@local.lookup_field}}

->
"Hello+' '+[field[local.lookup_field]]"
or
`Hello ${field[local.lookup_field]}`

Description

Re-implementation of #2388, to use set_data action globally and update set_item / set_items actions in data_items list to call the global function.

It also includes better test coverage, and support for items with dynamic expressions, such as global `set_da

Author Notes

General Use
Set values on a specific data_list item

click | set_data | _list_id: example_list, _id: example_id, completed:true, count: 1;

Can include @item reference that will use existing row context

click | set_data | _list_id: example_list, _id: example_id, count: @item.count + 1;

Can set item by _index

click | set_data | _list_id: example_list, _index: 0, completed:false;

Can target all items in a list (no current support for filtered subset) by omitting _id

click | set_data | _list_id: example_list, completed: true;

Dev Notes

Git Issues

Closes #

Screenshots/Videos

If useful, provide screenshot or capture to highlight main changes

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

Successfully merging this pull request may close these issues.

3 participants