fix(template-compiler): omit api_key for key outside iteration

[nolanlawson]

Details

Partially addresses #2697.

The idea is that if a template is like this:

<template>
  <x-foo key={myKey}></x-foo>
</template>

Then we should not render it with api_key, but instead ignore the key.

How we currently render it:

function tmpl($api, $cmp) {
  const { k: api_key, c: api_custom_element } = $api;
  return [
    api_custom_element("x-foo", _xFoo, { key: api_key(0, $cmp.myKey) }),
  ];
}

This PR:

function tmpl($api) {
  const { c: api_custom_element } = $api;
  return [api_custom_element("x-foo", _xFoo, { key: 0 })];
}

This PR also emits a warning if we find a key outside of an iteration.

Does this pull request introduce a breaking change?

• ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

• ✅ No, it does not introduce an observable change.

Almost everyone using a key outside of an iteration is probably doing it as a mistake, and even if they are, it should only impact the internals of our vnode-diffing engine.

GUS work item

W-10720220

[nolanlawson]

This code is now unnecessary! And we have a test to prove it.

Previous discussion: #2677 (comment)

[pmdartus]

I don't think it makes sense to keep the key directive as part of the AST if it doesn't carry any semantic.

[nolanlawson]

You're right. Fixed.

[pmdartus]

You can use warnOnNode instead of warnAtLocation this would avoid having to pass the parse5ElmLocation.

Suggested change

	ctx.warnAtLocation(
	ctx.warnOnNode(

[nolanlawson]

Thanks for the tip! Fixed.

[salesforce-nucleus]

❌ An unexpected error occurred while attempting to start the test stage of workflow 49905. Please try to start the stage again, or reach out to #nucleus-talk for help.

[nolanlawson]

/nucleus test

[nolanlawson]

/nucleus test

[nolanlawson]

lwc-platform downstream test failing due to Komaci test. Will look into it.

[nolanlawson]

So, it turns out there was a big use case we missed here – inline iterators and for:each:

<x-foo iterator:x={items} key={x.value} index={x.index} last={x.last} first={x.first}></x-foo>

<x-foo for:each={items} for:item="item" key={item.id}></x-foo>

In this case, the key can appear directly on the same element with for:each or iterator:x. We weren't handling this situation properly. I added a test and fixed this.

[nolanlawson]

@jmsjtu @pmdartus This part was really tricky – we had slightly different logic for getForOfParent and getForEachParent. It was causing getForOfParent not to work with inline iterators, because I was ignoring the key in that case.

I guess this didn't matter much before, because these tests were just looking for an error case (keys should not reference the index, or iteration missing a key entirely).

In my fix, we don't care if the for:each or iterator:x is on the same node or the parent node. I'd appreciate if you took a look to make sure I didn't miss something.

[nolanlawson]

...Just confirmed that it's possible to use an index as a key if the iterator is inline. My PR fixes this.

I can open it as a separate PR if you prefer. This one is getting pretty complex.

[nolanlawson]

Opened a separate PR: #2703

[nolanlawson]

This test passes in master. On 236 (v2.5.13) and 234 (v2.2.12), it throws an error:

Error: Invariant Violation: Compiler should produce html functions that always return an array.

The reason for the error is that the tmpl function is returning api_custom_element rather than api_iterator. So it looks like previously we didn't actually support inline iterators, only inline for:each.