Signal does not work along well with iterated components that do not have key attribute

[abdullahtellioglu]

Describe the bug

Page render throws Rendered fewer hooks than expected. This may be caused by an accidental early return statement. exception if your view has a signal definition and iterated components do not have keys. Internal conversation: https://vaadin.slack.com/archives/C0743B4BD5K/p1716545183155989

Expected-behavior

Not using the key property is something react warns user about. It is not mandatory to use it so signals should work with non-key attributed iteration. A better warning message would also be acceptable in my opinion since not using key is not recommended.

Reproduction

Add following code to your view and do some file changes to render the page.

import { ViewConfig } from '@vaadin/hilla-file-router/types.js';
import { useSignal } from '@vaadin/hilla-react-signals';
import { Button } from '@vaadin/react-components/Button';
import { Notification } from '@vaadin/react-components/Notification';
import { TextField } from '@vaadin/react-components/TextField';
import { HelloWorldService } from 'Frontend/generated/endpoints.js';

export const config: ViewConfig = {
  menu: { order: 0, icon: 'line-awesome/svg/globe-solid.svg' },
  title: 'User dashboard',
};

export default function UserdashboardView() {
  const name = useSignal('');

  return (
    <>
      <div className="grid grid-cols-6 gap-[1px] justify-center items-center">
        {Array.from({ length: 36 }, () => (
          <Button style={{ width: '50px', height: '50px' }}>{String.fromCharCode(65 + Math.floor(Math.random() * 26))}</Button>
        ))}
      </div>
    </>
  );
}

System Info

<properties>
        <java.version>21</java.version>
        <vaadin.version>24.4.0.beta4</vaadin.version>
    </properties>

[Legioth]

Simplified reproduction case:

import { useSignal } from '@vaadin/hilla-react-signals';

export default function EmptyView() {
  const s = useSignal('signal');
  return <>
    {[1].map(x => <span>{x}</span>)}
  </>
}

It seems like this issue only happens in association with HMR and only for some types of changes.

It happens if you load the view, and then change the array to [1, 2] but it's fine when reloading the page (and the warning about missing the key is then also shown in the console). It doesn't happen if you do some other change, e.g. updating the loop content to <span>{x}2</span> whereas changing it to <span>{x * 2}</span> triggers the issue. The issue cannot be reproduced if the useSignal is not present.

[cromoteca]

Does adding a key solve the issue? I think that keys are necessary when iterating in React.

[abdullahtellioglu]

It is not mandatory. React prints warning in dev mode. The key is used for performance optimization to check if the node is updated. https://react.dev/learn/rendering-lists. Adding a key to elements solves the issue.

[cromoteca]

Well, that documentation also states that

JSX elements directly inside a map() call always need keys

[Legioth]

It's irrelevant whether key is documented as required even though that requirement is not strictly enforced. What matters is that the error message gives no indication that the cause of the problem is that key is missing.

That's also why I'm not sure about the Impact: Low label here. Everyone does occasionally forget to add a key and when that happens, you have no idea about what's wrong due to this issue and might spend lots of time debugging and maybe even giving up.

[stefanuebe]

For me this worked as a workaround, but not sure if this is problematic with items, that have no id yet. Maybe the array index would be better here?

{Array.from(model.mySubItems, ({model: sModel, value}) => <MyFormPart key={value?.id} model={sModel}/>)}

[Legioth]

Maybe the array index would be better here?

The whole point of the key prop is so that the rendering logic in React would have something more than just the index to guide incremental updates. The key is used to identify if there's an existing DOM element to re-use rather than throwing away the old and creating a new one.

[abdullahtellioglu]

The exception happens when I add a key to a loop item and the exception is unclear about what goes wrong

Screen.Recording.2024-10-04.at.15.22.49.mov

[abdullahtellioglu]

The impact of this issue is very high, the app is unusable, and there is no clear answer as to why from user's perspective

[emarc]

I just hit this issue, and for a React novice trying to use Hilla which has practically no docs on how to do simple things like loop over some list it's a complete 🤯 :headbang: timewaste with an error that takes you on the wrong track completely.

You can code the thing, and it works, but when you touch the code or use Copilot to e.g align a thing it explodes.
IMO especially bad because it seems to work, but explodes under special circumstances.