Fix <FilterForm> ignores key prop on children

[Dreamsorcerer]

Tangentially related to #9587, if 2 inputs are used in filters with the same source then they end up with duplicate keys even when those components already provide their own unique keys.

[fzaninotto]

Can you please add a unit test showing the bug, and explain the use case for this? #9587 gives a good use case for datagrid fields, but I fail to understand in which case you need the same for filters.

[Dreamsorcerer]

explain the use case for this

If you have a FK that links to 2 different resources, you could have 2 ReferenceInputs in order to filter based on one linked resource or the other (you might even want a plain NumberInput or similar to set the value directly). Another case might be something like a combobox, radio buttons or similar to select from a number of favourite/recent options, while also having a text or slider or similar to be able to filter on other values.

[Dreamsorcerer]

Can you please add a unit test showing the bug

I put a test in, but realised the key is not rendered, so I don't think there's anything I can actually assert with the test.

Not sure how you handle warnings, but I have jest-fail-on-console on my own test suite to fail on warnings. This code should produce a warning from React without the fix (due to having duplicate keys).

[fzaninotto]

I'm still not convinced by the use cases you list. It may be something you want to do in your app, but I don't think it's something we want to support with the react-admin basic filters. I'll wait for the feedback of other contributors.

Your test doesn't seem to do any useful assertion. Also, I understand that the filter already works in your use case, but simply logs a warning in the console. Am I right?

[Dreamsorcerer]

It may be something you want to do in your app, but I don't think it's something we want to support with the react-admin basic filters.

I mean, it's not like I'm asking you to explicitly support these complex features. I just want to be able to specify the key on components explicitly, as I've done everywhere else in the app. It seems odd to me that it throws away the key that already exists on the component, and the fix for that is 2 words.

Your test doesn't seem to do any useful assertion.

That is what I said. The only thing I can test, as far as I can tell, is that the warning doesn't occur.

[Dreamsorcerer]

Also, I understand that the filter already works in your use case, but simply logs a warning in the console.

Just to expand on that, this is the warning. It's pretty clear that the behaviour is bad, may cause issues today, and may become a error in future:

Warning: Encountered two children with the same key, fk_child_id__ref_num. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version.

[Dreamsorcerer]

I've updated the test so it's explicitly testing something now.

Another case might be something like a combobox, radio buttons or similar to select from a number of favourite/recent options, while also having a text or slider or similar to be able to filter on other values.

A specific example of this is filtering on age range. You might have one input that provides a 2-point slider to select a range. Then a second input that provides a drop-down of pre-selected age ranges for convenience ("Adult (18+)", "Child (3-17)", "Infant (<=2)").

[fzaninotto]

Your test passes even without the fix in FilterForm... It's not testing the right code.

[Dreamsorcerer]

Sorry, I've been super busy. If the test is still passing, then I can only imagine the tests are already doing something with warnings and managing to ignore them?

[fzaninotto]

Not that I know of.

[Dreamsorcerer]

Not that I know of.

OK, it's an error, not a warning. Test works properly now. FYI, there are plenty of other errors and warnings getting logged in the other tests.

[slax57]

Thanks!