[EuiInMemoryTable] Misleading documentation

[sebelga]

trafficstars

The documentation of the <EuiInMemoryTable /> onChange prop says

Called whenever pagination or sorting changes (this property is required when either pagination or sorting is configured). 

But the implementation does not enforce it. No error is thrown if pagination is provided but no onChange handler.

In https://github.com/elastic/kibana/pull/132321 I've added the onChange prop (here). As you can see the pagination prop was already declared but the component did not throw an error previously when the onChange was missing. Furthermore the pagination state was handled internally by the component before the onChange prop was added (even though the docs say this property is required when either pagination or sorting is configured).

This is a bit confusing from a consumer point of view.

I would suggest to

• Throw an error if either the pagination or the sorting prop is provided without the onChange prop
• Add a new defaultPagination prop to provide the initial value of the pagination that will then be handle internally by the component. This will differentiate the "controlled" vs "non-controlled" state behaviours.

[thompsongl]

I think that bit of documentation is just incorrect 😅

Our own example has both pagination and sorting configured and does not use onChange. As you mention, each state will be handled internally if not configured otherwise. We should be able to just remove the parenthetical clause.

[sebelga]

@thompsongl Could you add a dummy onChange prop to your example and see if it still works?

<EuiInMemoryTable
      tableCaption="Demo of EuiInMemoryTable"
      items={store.users}
      columns={columns}
      pagination={true}
      sorting={sorting}
      onChange={() => undefined} // add this
    />

[thompsongl]

Could you add a dummy onChange prop to your example and see if it still works?

Pagination and sorting state are no longer handled internally when onChange is a noop. So we should update the docs to indicate that onChange is not required when pagination and/or sorting are configured, but if onChange is present it is responsible for handling state for each/both.

[sebelga]

Thanks, yes that would be great thanks 👍

I know that it would mean a breaking change (so probably not worth the change) but ideally having different props for different behaviours (controlled vs non-controlled state) help from a consumer point of view.

In this case having defaultPagination and defaultSorting props clearly indicate that this is not controlled by the consumer and that the state is internally managed.

[github-actions[bot]]

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

[github-actions[bot]]

❌ We're automatically closing this issue due to lack of activity. Please comment if you feel this was done in error.