don't allow call binding to affect the overrideContext

[ccantill]

We ran into an issue where in case of a naming collision in the call args with properties on the overrideContext, those properties would disappear from the overrideContext. This makes sure that doesn't happen.

[bigopon]

@ccantill thanks for this PR. I'm tempted to say this is quite a behavior that we should discourage. Maybe we should discourage this and encourage folks to access everything inside $event. Doing it the assign way potentially has issues with identity of the value as well. I'm proposing this:

    let overrideContext = this.source.overrideContext;
    if (!Call.noLegacy) {
      Object.assign(overrideContext, $event);
    }
    overrideContext.$event = $event;
    let mustEvaluate = true;
    let result = this.sourceExpression.evaluate(this.source, this.lookupFunctions, mustEvaluate);
    delete overrideContext.$event;
    if (!Call.noLegacy) {
      for (let prop in $event) {
        delete overrideContext[prop];
      }
    }
    return result;

[bigopon]

cc @fkleuver @EisenbergEffect

[ccantill]

I agree that this behaviour is somewhat dubious, but that's how it is documented. I don't think using it should ever potentially affect the parent overrideContext however.

Doing it the assign way potentially has issues with identity of the value as well.

Not sure what you mean?

[bigopon]

It's related to how assignment works: we assign values, not the descriptors of those values. So if someone have:

$event = {
  get arg1() {
    return this.calcuate(...);
  }
}

Then the arg1 getter would needs to be invoked because Call binding needs to resolve it at the assignment, instead of when it's actually read. Won't be a big deal most of the time, but we should avoid it.

May I know where in the doc did you see this? We probably should add some note advising against it?

[ccantill]

I don't think my patch changes that behavior; it's currently already invoking the getter in the Object.assign(overrideContext, $event); line. The only real difference is that instead of reusing and modifying the existing overrideContext it creates a new object.

It's documented in https://aurelia.io/docs/binding/basics#dom-events under "Function References":

For example, invoking the function with this.go({ x: 5, y: -22, z: 11})) will make x, y and z available for binding: <my-element execute.call="doSomething(x, y)"></my-element>

[bigopon]

@ccantill I didn't mean the current patch change this behavior, I only meant to say they are a bit of an unexpected behavior. Reading the doc, I realize what they are for, thank you. I'm still not sure if we should maintain this behavior in v2, maybe it's better that things are accessed in the $event object itself I think.

For the patch, I think there's an edge case with assignment in the call, something like this:

<my-el task.call="value = doSomething(x, y)"></my-el>

and if value is on the existing override context, the assignment won't assign the value to the correct object. It's a bit of a dilemma, because I don't think anyone is using .call this way.

[ccantill]

I'm not sure assigning in the call expression would result in the expected behavior anyway; I would expect that to set the property on the viewModel, but wouldn't this only change the overrideContext and leave the viewModel unaffected?

[bigopon]

Say we are inside this template

<let value.bind="some.expression"></let>

<my-el task.call="value = doSomething(x, y)"></my-el>

... other stuff that binds to `value` of override context

value is from existing override context, created by <let/> binding. And in our expression, read/write will deal with override context first, before binding context, hence a bit of inconsistency if someone was doing it this way.

You could be right that doing an assignment in call could be odd. I think let's get @fkleuver and @EisenbergEffect /others to chime in a bit?