render() eventContext is not passed to event parts correctly when bound to same prototype function

[Tundon]

Description

When calling render(template, container, { eventContext }), if the event is bound to a function that belongs to a prototype (class member function), even eventContext is changed, the handler will still be invoked against the initial eventContext

Steps to Reproduce

1. Write this test case in file src/test/lib/render_test.ts

// ...

suite('render()', () => {
  // ...
  suite('events', () => {
    // ...

    test('uses correct eventContext', () => {
      class Context {
        count = 0;
        handler() {
          this.count++;
        }
      }
      
      const t = (context: Context) => html`<div @click=${context.handler}></div>`
      
      const context1 = new Context
      const context2 = new Context
      render(t(context1), container, { eventContext: context1 as unknown as EventTarget })
      render(t(context2), container, { eventContext: context2 as unknown as EventTarget })
      
      const div = container.querySelector('div')!;
      div.click();
      assert.equal(context1.count, 0);
      assert.equal(context2.count, 1);
    })
  });
  // ...
})

2. see the output of tests

Expected Results

The test case should pass

Actual Results

The test case fails

Browsers Affected

• Chrome
• Firefox
• Edge
• Safari 11
• Safari 10
• IE 11

[csvn]

I think your example is incorrect? I simplified it a bit below. At line (1) the code is saying you want this to be context1. This means that when the handler is executed, it's context1s count that is incremented, not context2.

  test('uses correct eventContext', () => {
    function handler() { this.count++; }
    const t = (context: Context) => html`<div @click=${handler}></div>`
      
    const context1 = { count: 0 };
    const context2 = { count: 0 };
    render(t(context1), container, { eventContext: context1 });
    render(t(context2), container, { eventContext: context1 }); // (1)
      
    container.querySelector('div')!.click();

    assert.equal(context1.count, 0);
    assert.equal(context2.count, 1);
  });

I.e., with the code in your example above, it should be:

    assert.equal(context1.count, 1);
    assert.equal(context2.count, 0);

[justinfagnani]

I think @csvn is right here. @Tundon did you mean to set the eventContext to context1 in both cases?

[Tundon]

No, I made a mistake in the code, and updated that. The second render is supposed to use context2, the idea is: both render call is binding to the same function, but with different eventContext.

The reason is that, when rendering, the part would find the bound handler doesn't change, and the changed eventContext is not passed down to event part at the second render call, thus it would be invoked against old eventContext.

[justinfagnani]

Ok, thanks for the update.

Can you describe your use case?

The eventContext is read when we create the event binding and indeed never updated again. It's meant for cases like LitElement where the context of its template will never change over time. Other render options we use like the shady CSS scope aren't allowed to change over time. It's be some work to allow this.

[Tundon]

Alright, so my use case may be a bit complicated and not common, but I try my best to explain.

So I have a custom element (not extending from LitElement), PageContainer, which has a property controller. This controller property would have a template getter to generate template result:

interface Controller {
  readonly template: TemplateResult;
}

class PageContainer {
  public controller: Controller | null = null;
}

The purpose for PageContainer is to render template (into itself) dynamically based on injected controller instance. And there are cases I need to reload the PageContainer's content for controller instances that are from same class, that is how I found this issue.

@justinfagnani I understand the intended use case for eventContext, and I am not sure how popular my use case would be, if the decision is not to consider this type of use case, update the doc to mention that the renderOptions is one time use per target would be good.

The workaround for my use case is just to always use the form @event=${() => handler()}, so that this context is always embedded in the bound value.

[kevinpschaaf]

It's probably fair that RenderOptions is not the best name here, because it's really only respected on first-render, not on updates.

We do document this fact in the guide:

Render options should not change between subsequent render calls.

It's also noted in the API docs in source, but it looks like these aren't being carried to the API docs on the website, so we'll look into that (filed as #1177).

The workaround you came up with (re-binding the function every render) seems like a reasonable tradeoff. Changing the API to give a guarantee that any change to RenderOptions will invalidate all prior rendering is probably not a change we're going to consider at this time, so going to go ahead and close this issue.