Improve DataForm performance

[gigitux]

What?

While investigating the DataView/DataForm related to WooCommerce issue #50992, I found that the dataform component in the regular view re-renders in its entirety whenever the onChange function is triggered. To enhance performance, this PR addresses the issue by ensuring that only the specific input that has changed is re-rendered, rather than the entire component.

Why?

This PR is necessary because re-rendering the entire component on every change can lead to performance bottlenecks, especially in long forms or those with slow components. For example, I simulated a slow edit component for the title field and observed significant performance improvements with this patch compared to without it.

You can verify that even the title field component is slow, this doesn't impact the speed of the other fields.

Before	After
Screen.Capture.on.2024-09-03.at.14-13-11.mp4	Screen.Capture.on.2024-09-03.at.14-14-27.mp4

Testing Instructions

1. Checkout trunk
2. Edit the packages/dataviews/src/dataform-controls/text.tsx file adding this function at top of the file:

function slowFunction() {
	// Simulate a CPU-intensive task
	const start = performance.now();
	while ( performance.now() - start < 1000 ) {
		// This loop runs for 1 second, simulating a slow computation
	}
	return;
}

In the Text component invoke this function:

export default function Text< Item >( {
	data,
	field,
	onChange,
	hideLabelFromVision,
}: DataFormControlProps< Item > ) {
	const { id, label, placeholder } = field;
	const value = field.getValue( { item: data } );

	slowFunction();

	const onChangeControl = useCallback(
		( newValue: string ) =>
			onChange( {
				[ id ]: newValue,
			} ),
		[ id, onChange ]
	);

3. Run Storybook ( npm run storybook:dev).
4. Check the DataForm story.
5. Try to update the title input.
6. Ensure that the title input is slow.
7. Edit other fields.
8. Expect that other fields are slow.
9. Checkout this branch (keep the changes)
10. Check the DataForm story.
11. Try to update the title input.
12. Ensure that the title input is slow.
13. Edit other fields.
14. Expect that other fields aren't slow

Testing Instructions for Keyboard

Screenshots or screencast

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: gigitux <gigitux@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[youknowriad]

What if instead of passing data we pass field.getValue( { item: data } ) and we just memoize FieldEdit simply.

[gigitux]

Thanks for the review! You raise a great point. I'm not entirely sure—could it be that the data object might be useful for more complex edit field components?

[youknowriad]

could it be that the data object might be useful for more complex edit field components?

If it's the case, the current memoization will break it because it won't update if something else changes.

[gigitux]

Yeah, you're right! I'm inclined to follow with your suggestion and revisit this approach if/when needed: WDYT?

[youknowriad]

Works for me.

[youknowriad]

This is looking good to me. Can we add a "CHANGELOG" note about it. I'm guessing this changes the API of custom Edit functions, so it's a breaking change.

I'm curious about @oandregal's thoughts here as well.

[gigitux]

This is looking good to me. Can we add a "CHANGELOG" note about it. I'm guessing this changes the API of custom Edit functions, so it's a breaking change.

I noticed that it is necessary the callback to the memo function: the issue is the field that is an object.

I'm curious about @oandregal's thoughts here as well.

I’m happy to close or put this PR on hold and revisit it later if you feel it’s a premature optimization.

[youknowriad]

I'm not sure we should be doing this though. This would prevent the component from re-rendering if field or onChange change.

[gigitux]

Yeah, true! I realize that finding a good fix might be more challenging than I initially thought. As I mentioned earlier, I'm okay with closing this PR for now, but we should definitely revisit this issue later. Large forms could eventually suffer from performance issues, so it’s worth addressing.

[youknowriad]

No for me this PR is good.

• I don't expect "field" to change over time
• I'm not sure what we're doing right now with "onChange" but we can easily pass a stable reference that calls the latest prop.

So I think we can consider memoizing the fields like you did with a simple memo call. That said, in general I like to consider metrics before doing performance improvements. If we notice a real issue, we can add a metric to code vitals.run to ensure we keep that good over time.

[oandregal]

I don't expect "field" to change over time

One situation where this will happen is when the elements of one field depend on another field. Think of countries & regions situation, for example.

[youknowriad]

One situation where this will happen is when the elements of one field depend on another field. Think of countries & regions situation, for example.

I don't think it's the "field" that changes in this situation, we will need access to the whole "data" though yes and not just the current item.

[oandregal]

I don't think it's the "field" that changes in this situation, we will need access to the whole "data" though yes and not just the current item.

🤔 Aren't the filter elements defined by the Field? When a user changes the country to a different one, the region field would need to recreate their element list, wouldn't it?

[youknowriad]

Aren't the filter elements defined by the Field? When a user changes the country to a different one, the region field would need to recreate their element list, wouldn't it?

I think we need a better way to address this use-case. Maybe the elements as a function or something like that. I think field declaration should ideally be "static" mostly.

[oandregal]

So, the core change here is decoupling components from data changes. That sounds good. However, what if the value is an object or array? Wouldn't we have the same issue? I'm thinking of fields that represent a list of categories, for example.

API-wise, I'm not strongly opposed to doing something like this where the derived state (value) is calculated outside the Edit — though I don't love it. Are there alternatives we can consider that don't collocate derived state away from Edit? 🤔 Does it make sense to pass a getData stable reference instead?

[youknowriad]

However, what if the value is an object or array? Wouldn't we have the same issue? I'm thinking of fields that represent a list of categories, for example.

This has no impact, the instance of that object or array won't change when you edit another field