Throw on duplicate keys in iterators

[wjhsf]

The for:each and iterator directives require that the iterated child has a key attribute in order to guarantee proper rendering/reactivity. However, the code that validates the key attribute does not throw on invalid or duplicate keys. Instead, it simply logs an error in development mode, and does nothing in production mode.

lwc/packages/@lwc/engine-core/src/framework/api.ts

Lines 488 to 492 in e0029ee

	if (process.env.NODE_ENV !== 'production') {
	if (!isUndefined(iterationError)) {
	logError(iterationError, vmBeingRendered!);
	}
	}

Although the framework warned against it, having duplicate keys mostly still worked as expected, for a long time. Recently, however, we introduced the lwc:on directive (#5344). A code path was introduced that assumes that a VM has an elm property that is always defined. However, when duplicate keys are used, this is not always the case, and an error is thrown due to an invalid weak map key.

lwc/packages/@lwc/engine-core/src/framework/modules/dynamic-events.ts

Line 96 in e0029ee

vm.attachedEventListeners.set(elm, attachedEventListeners);

This does not always occur; we have not identified the root cause. Nevertheless, duplicate keys are invalid and now cause an actual error to occur, so we should make the error happen at the source, i.e. we should throw the duplicate key error rather than simply log it. This is a breaking change, so it must be done with API versioning.

With the new behavior enabled, the iterationError should always be thrown, regardless of NODE_ENV. On older API versions, the error should be a warning, rather than an error, because it is not thrown. It should also be logged regardless of NODE_ENV, and the message should be updated to include a notice that the behavior will change when components upgrade to the relevant API version.

Update: this isn't a safe change, so we've narrowed the scope. See #5540 (comment).

[git2gus]

This issue has been linked to a new work item: W-19972730

[gaurav-rk9]

This issue occurs in non-special case of updateDynamicChildren

If 2 new vnodes have the same key, they both would map to the same idxInOld. for the first vnode patching, elmToMove is a vnode and hence it will patch normally and set oldCh[idxInOld] to undefined. Now for the 2nd vnode, idxInOld will remain the same and hence elmToMove will be undefined , and since there is no else block, it silently moves to next vnode without doing anything for the current vnode.

Now during the next patching, idxInOld would correspond to the last old vnode with that key, and as described above, it didn't undergo any patching previously, so its elm will be undefined. So the error from patchDynamicEventListeners occurs during the patching of first new vnode with that same key

[gaurav-rk9]

Also, An actual error was thrown previously also, during other patches (props, attrs, etc...). It's just that now patchDynamicEventListeners is the first patching function.
Remove this line and observe

lwc/packages/@lwc/engine-core/src/framework/rendering.ts

Line 590 in e0029ee

patchDynamicEventListeners(oldVnode, vnode, renderer, owner);

[gaurav-rk9]

see https://stackblitz.com/edit/salesforce-lwc-cjgdfveb?file=src%2Fmodules%2Fx%2Fapp%2Fapp.js for an example.

[wjhsf]

Thanks for figuring out where the undefined is coming from!

Also, An actual error was thrown previously also, during other patches (props, attrs, etc...). It's just that now patchDynamicEventListeners is the first patching function.

see https://stackblitz.com/edit/salesforce-lwc-cjgdfveb?file=src%2Fmodules%2Fx%2Fapp%2Fapp.js for an example.

The framework is misleading. The "Duplicated "key" attribute value" error is never thrown, it's just logged. In the example you've provided, there aren't any thrown errors that disrupt the rendering engine.

The actual thrown errors that we've seen as a result of duplicate keys don't actually occur as part of the rendering of the list. They occur when a re-render is triggered on an element in a different part of the tree. I haven't verified, but I suspect that the element that throws the error may be a nested child of the list item element.

[wjhsf]

Just sharing a playground that did not reproduce the error.

[gaurav-rk9]

The framework is misleading. The "Duplicated "key" attribute value" error is never thrown, it's just logged. In the example you've provided, there aren't any thrown errors that disrupt the rendering engine.

Yes, I am not disputing that. I agree that framework should throw early as stated in OP.
My assertion is that, In whichever cases we see an error "thrown" today, we would be seeing an "thrown" error even if lwc:on had not been introduced and an prop, attr or eventListener is modified in the looped element.
Lots of other patching functions (atleast props.ts, attrs.ts, events.ts) also assume that elm property is defined.
This can be verified by removing the following line in playground and click "Change Bar" button twice.

lwc/packages/@lwc/engine-core/src/framework/rendering.ts

Line 590 in e0029ee

patchDynamicEventListeners(oldVnode, vnode, renderer, owner);

The actual thrown errors that we've seen as a result of duplicate keys don't actually occur as part of the rendering of the list. They occur when a re-render is triggered on an element in a different part of the tree

In the provided example, it does occur when the list elements are patched. Not on the first render, but on the third render. I forgot to write steps to repro - you need to click "Change Bar" button twice.

The error is reproducible in some specific subset of cases that could occur in updateDynamicChildren, and needs at-least 2 re-renders after first render.

[wjhsf]

Thanks for the clarification on the repro!

My assertion is that, In whichever cases we see an error "thrown" today, we would be seeing an "thrown" error even if lwc:on had not been introduced and an prop, attr or eventListener is modified in the looped element.

Yep, we've seen that as well. The lwc:on code path just happened to be the code path that was hit in the specific bugs we encountered, which made us realize that we should update the framework to fix all the code paths by addressing it sooner.

[gaurav-rk9]

Also, Now that I think of it, we should delay call to getAttachedEventListeners, until it is actually needed, unconditionally of API version. This is what allows props, attrs, event patch functions to not throw in many cases becuase they don't access elm until it is actually needed.
This is essential, so that we don't break customers who have been using their components just fine, until now, even though it was invalid.
This is in addition to whatever is proposed in OP

[wjhsf]

Upon reflection, this change is not intrinsically observable, but the changed behavior can trivially be exposed. A component may bind for:each to an @api-decorated prop, which would delegate responsibility for setting the key to another component. For example, consider a wrapper component like an image carousel or button group.

After discussion, we have decided to restrict the scope of this change to simply changing the error text to include something scary (e.g. "THIS WILL CAUSE RENDERING ERRORS") and to also log it in production. (We will continue to log an error, rather than warning, as developers tend to ignore warnings.)