Closed winged closed 4 years ago
You're 100% correct. There's another issue in here that would have benefitted from that also, so I've been playing with solutions. I don't want to give up the ability for binary operators to evaluate the left and right sides in parallel (this is a massive speed improvement for users of async transforms), so I'm toying with some binaryOp-level config that will specify whether a given binary op should use parallel or synchronous processing. Stay tuned :)
I'm toying with some binaryOp-level config that will specify whether a given binary op should use parallel or synchronous processing. Stay tuned :)
Hmm, I wonder if some specific syntax for parallel processing wouldn't be better. Having "core" features of a language configurable feels very.. PHP-ish to me (personal opinion) and may lead to unexpected behaviour, as the users writing JEXL expressions may not be the same ones who configure the runtime.
Instead, for example you could have a parallelMap()
and let users who want parallel processing encode ||
and &&
via any(parallelMap(f,[a,b]))
and all(parallelMap(f,[a,b]))
Oh, I didn't mean expression-facing option :). I just meant a flag in the grammar. Consider the following-- this is a snippet from the grammar file as it is now:
'<=': {
type: 'binaryOp',
precedence: 20,
eval: (left, right) => left <= right
},
'&&': {
type: 'binaryOp',
precedence: 10,
eval: (left, right) => left && right
},
I'm simply playing with this idea:
'<=': {
type: 'binaryOp',
precedence: 20,
eval: (left, right) => left <= right
},
'&&': {
type: 'binaryOp',
precedence: 10,
manualOperandEval: true,
eval: async (left, right) => await left.eval() && await right.eval()
},
I'd just expose the manualOperandEval
flag as an option in addBinaryOp
and addUnaryOp
and we'd be all set. There'd be no such thing as a parallel-processed or configurable &&
and ||
.
I'm also considering making this a breaking change and force all operators to call .eval()
on left and right, but that's a hard break. Harder if I do it on the transforms as well for consistency. Plus, the simplicity of the pre-eval behavior is super nice and makes Jexl a bit more approachable. ...but it would be nice to eliminate the option. Will chew on this a bit more.
Small amendment... after some thought, for TypeScript friendliness I'll likely axe the boolean flag and instead split eval
into two different keys: one for manual resolution, the other for the existing functionality. Assuming I don't go with the all-manual solution I'm still considering.
I do like that idea! :+1:
Boom, see #84 for the feature. The syntax in the grammar file isn't as nice as what I proposed above because that wouldn't have supported synchronous evaluation, but I'm happy with where it landed.
I decided to do this in two steps. Step 1 is releasing the functionality as a non-breaking change. &&
and ||
now evaluate the right operand conditionally, and jexl.addBinaryOp
has a new optional argument to denote whether your custom binary operator will be responsible for calling eval
on its own operands. This can be released now without a major version bump.
Step 2 will come whenever this library embraces Typescript a bit better, because changing the type of arguments depending on an unrelated boolean is type hell. So with a major version bump, I'm planning to change addBinaryOp
to accept an object similar to the objects in the grammar file instead of 4 different arguments.
Many if not most programming languages implement shortcuts for the logic operators
or
andand
:or
, if the left hand parameter istrue
, the right hand parameter is not evaluated as the result is already determined to betrue
and
, if the left hand parameter isfalse
, the right hand parameter is not evaluated, as the result is already determined to befalse
This allows for some checking to see if for example a calculation even makes sense (think avoiding division by zero for example):
foo != 0 && 3 / foo < 10
).As I understand the code (https://github.com/TomFrost/Jexl/blob/master/lib/evaluator/handlers.js#L27), this is currently not implemented, causing every "branch" of an expression being evaluated, even if it's not needed to calculate a result.
Could we consider implementing this change?