Fix <ListView> should show custom empty component with partial pagination

[yanchesky]

The logic that checks whether the custom empty component should be shown relies on the total value. Since react-admin supports partial pagination, it should handle that case too. This PR fixes that.

[slax57]

Nice catch, thanks!

[yanchesky]

Removed propTypes as you asked. I'm not sure though if I did not remove too many of them. Some of them are passed to the useListContext. I left only those which are directly consumed by

Also, I fixed the condition you proposed as it broke the test. Btw what's wrong with using optional chaining?

[slax57]

Thanks for cleaning out the propTypes 🙂

Regarding the condition, it was not about the conditional chaining which is perfectly fine to use.
It's just that I would expect the empty page to be displayed also in case data = undefined.

With the original condition, when data = undefined, data?.length === 0 will return false, and so the empty page will not be displayed, which is not what I'd expect.

Hence I suggested (!data || data.length === 0), which will return true in this case.

Note that your latest version !!data && data.length === 0 also won't work as it returns false.

Can you fix it?
Thanks 🙂

[yanchesky]

In that case, your suggestion indeed makes sense. However, as I said, returning a custom component in case data = undefined or null breaks the test. Here is failed test run and below is the implementation of the test that fails. We can see that the returned response contains only error and the assertion is to render the error component.

react-admin/packages/ra-ui-materialui/src/list/List.spec.tsx

Lines 228 to 248 in 83aa214

	it('should render a list page with an error message when there is an error', async () => {
	jest.spyOn(console, 'error').mockImplementation(() => {});
	const Datagrid = () => <div>datagrid</div>;
	const dataProvider = {
	getList: jest.fn(() =>
	Promise.reject({ error: { key: 'error.unknown' } })
	),
	} as any;
	render(
	<CoreAdminContext dataProvider={dataProvider}>
	<ThemeProvider theme={theme}>
	<List resource="posts">
	<Datagrid />
	</List>
	</ThemeProvider>
	</CoreAdminContext>
	);
	await waitFor(() => {
	expect(screen.getByText('ra.page.error'));
	});
	});

The error component is rendered by the renderList function rather than renderEmpty, thus we intend shouldRenderEmptyPage to be false when data = undefined.

react-admin/packages/ra-ui-materialui/src/list/ListView.tsx

Line 97 in 83aa214

{shouldRenderEmptyPage ? renderEmpty() : renderList()}

react-admin/packages/ra-ui-materialui/src/list/ListView.tsx

Lines 54 to 79 in 83aa214

	const renderList = () => (
	<div className={ListClasses.main}>
	{(filters || actions) && (
	<ListToolbar
	filters={filters}
	actions={actions}
	hasCreate={hasCreate}
	/>
	)}
	<Content className={ListClasses.content}>
	{bulkActionButtons &&
	children &&
	React.isValidElement<any>(children)
	? // FIXME remove in 5.0
	cloneElement(children, {
	bulkActionButtons,
	})
	: children}
	</Content>
	{error ? (
	<Error error={error} resetErrorBoundary={null} />
	) : (
	pagination !== false && pagination
	)}
	</div>
	);

IMO this is the correct approach. The case when isLoading = false and data = undefined seems like an error to me. Also, dataProvider expects data to be somehow defined, which validateResponseFormat.ts confirms. I would stay then with my first solution as it did the job. Any other solution would require more refactoring (Which I'm willing to do if you would like so 😊).

Also speaking of validateResponseFormat.ts a while ago I noticed that the error produced by this function is not accurate. This may be misleading for new users, because the actual response may be { data: [...], total: 123 } or { data: [...], pageInfo: {...} } . Would you like me to fix this in this PR, create a new one or leave it as is?

react-admin/packages/ra-core/src/dataProvider/validateResponseFormat.ts

Lines 52 to 61 in 83aa214

	if (
	fetchActionsWithTotalResponse.includes(type) &&
	!response.hasOwnProperty('total') &&
	!response.hasOwnProperty('pageInfo')
	) {
	logger(
	`The response to '${type}' must be like { data: [...], total: 123 }, but the received response does not have a 'total' key. The dataProvider is probably wrong for '${type}'`
	);
	throw new Error('ra.notification.data_provider_error');
	}

[slax57]

Okay. I think you're right. Thanks for the detailed answer 🙂
We should keep your initial implementation then. I'll revert my suggested change.

Regarding the issue with validateResponseFormat.ts, this is a good point.
This would deserve its own PR since it's not linked to this fix. Can you open a new one?
Thank you so much 🙂

[yanchesky]

Regarding the issue with validateResponseFormat.ts, this is a good point. This would deserve its own PR since it's not linked to this fix. Can you open a new one? Thank you so much 🙂

Sure. I'll do that today evening.