fix: prevent expressions from recomputing more than once #1154
Conversation
Skaiir
force-pushed
the
1151-batch-expression-field-evaluation
branch
from
1fca212
to
98f4159
Compare
Skaiir
changed the title
Skaiir
force-pushed
the
1151-batch-expression-field-evaluation
branch
from
98f4159
to
7519fdb
Compare
Skaiir
force-pushed
the
1151-batch-expression-field-evaluation
branch
from
7519fdb
to
4bebb52
Compare
packages/form-js-viewer/src/features/expressionField/ExpressionLoopPreventer.js
Outdated
Show resolved
Hide resolved
packages/form-js-viewer/src/features/expressionField/ExpressionLoopPreventer.js
Outdated
Show resolved
Hide resolved
packages/form-js-viewer/src/features/expressionField/ExpressionLoopPreventer.js
Outdated
Show resolved
Hide resolved
|
@Skaiir Please check why the Carbonisation test is failing |
|
@vsgoulart It's the tasklist carbonisation that's failing because they are out of sync following the UX changes during spring cleaning. |
| eventBus.on('import.done', this.reset.bind(this)); | ||
| eventBus.on('reset', this.reset.bind(this)); |
There was a problem hiding this comment.
So are we saying here:
Expression fields will not recompute unless
- A field with
shouldNotRecompute: false | null | undefinedis updated - The form is reloaded
- The form is reset
There was a problem hiding this comment.
Basically yes, the idea being that we only want expressions to compute once when a change has happened, except if that change was triggered by an expression (in which case we ignore it).
Basically, that makes a change (say a form field value is set) propagate through every expression at most once, preventing them from re-evaluating in loops.
Ideally in the future, we don't have to do any of this sillyness when we separate view and model, but for now it was the best way I found to get around the looping issue entirely within the constraints. It's also why I built it as a module, so that it can be easily extracted.
There was a problem hiding this comment.
With this system, loops will still loop (they will advance one step per value change). But we are happy having loops be undefined behavior in general, as they are faulty configs. Like you mentioned, linting would be great, but it's hard.
There was a problem hiding this comment.
Note this does not prevent expressions from chaining, however it does give a certain top to bottom precedence to how they evaluate if the dependency graph has cycles. But again, if they do have cycles it's considered undefined behavior and not something we should care about.
There was a problem hiding this comment.
That's fine. I am ok with making loops only run once on a change. I do agree that we should separate out the model and the view but that's too big of a change for a fix.
| "components": [ | ||
| { | ||
| "computeOn": "change", | ||
| "type": "expression", | ||
| "id": "Field_exprA", | ||
| "key": "exprA", | ||
| "expression": "=exprB + 10" | ||
| }, | ||
| { | ||
| "computeOn": "change", | ||
| "type": "expression", | ||
| "id": "Field_exprB", | ||
| "key": "exprB", | ||
| "expression": "=exprA * 2" | ||
| } | ||
| ], | ||
| "type": "default", | ||
| "id": "Form_cyclic_example", | ||
| "executionPlatform": "Camunda Cloud", | ||
| "executionPlatformVersion": "8.5.0", | ||
| "exporter": { | ||
| "name": "Camunda Modeler", | ||
| "version": "5.22.0" | ||
| }, | ||
| "schemaVersion": 16 |
There was a problem hiding this comment.
Is there some way to creating a linting rule that warns the user about this cyclical reference?
There was a problem hiding this comment.
Yes there is, but it involves building up an object graph of all the feel expressions, the various local variable contexts, ect... Not having a separated model and view makes this extra difficult and not something I think warrants the time investment.
Closes #1151