Default shipping state, wp-data's `isResolving`, vs. `Form.Props.initialValues`

[tomalec]

This is a follow-up from #422 (comment)

Describe the bug:

The problem is visible at https://github.com/woocommerce/google-listings-and-ads/tree/2e5d58ac1f67f01ee33e076d03014c68ac38311d

What we do:

1. We have a default state of [] -

   google-listings-and-ads/js/src/data/reducer.js

   Lines 16 to 17 in 2e5d58a

   	rates: [],
   	times: [],

2. EditFreeCampaign fetches shipping rates from the server-side -

   google-listings-and-ads/js/src/edit-free-campaign/index.js

   Line 102 in 2e5d58a

   const { data: savedShippingRates } = useShippingRates();

3. EditFreeCampaign > SetupFreeListing > From renders given shippingRate.

Expected behavior:

From should eventually render data fetched from the server-side.

Actual behavior:

Sometimes, if racy flows align, the user gets the form with no shipping rates. Gets stuck with the initial data [].

Steps to reproduce:

1. Checkout https://github.com/woocommerce/google-listings-and-ads/tree/2e5d58ac1f67f01ee33e076d03014c68ac38311d
2. Go to https://gla1.test/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fedit-free-campaign&programId=123&pageStep=2

Additional details:

To me it looks like the problem is three fold:

Form

The first problem we have is that the <Form> component uses initialValues to create its own internal state, then ignores subsequent changes made to this prop. I believe it's a faulty implementation and something that should be addressed there.

From our (GLA) perspective fixing it there may take too long, as:

• this is an external component, with its own governance and release flow
• this is undocumented and untested behavior, therefore other plugins could rely on this as a feature

wp-data & isResolving

We should be able to solve that on our end by not rendering the Form until the data is ready.
But we cannot.

With the way we obtain the data, we cannot distinguish whether the data was already loaded or not.

google-listings-and-ads/js/src/hooks/useShippingRates.js

Lines 13 to 21 in 2e5d58a

	const { getShippingRates, isResolving } = select( STORE_KEY );
	const data = getShippingRates();
	const loading = isResolving( 'getShippingRates' );
	return {
	loading,
	data,
	};

The loading flag indicates whether the request is taking place, no whether we asked for it.
So from EditFeeCampaign or Form perspective, the timeline would look like this:

1. default state is []
2. we call const { data: savedShippingRates, loading } = useShippingRates();. savedShippingRates is [] (default), loading is false
3. a moment later savedShippingRates is [] (default), loading is true
4. once data comes savedShippingRates is [...], loading is false

(see https://github.com/woocommerce/google-listings-and-ads/tree/85612854603bd1d36f8d879d064af5d048109c11)
The problem is just by using those two variables savedShippingRates and loading we cannot judge whether it's the default state or the one fetched from the server-side. (empty array [] could be a valid saved state as well)

That means we may need to change the method we fetch the data, to expose information whether it was fetched or not.

Default

I also start to wonder whether [] is a good default/initial state. Before any data is loaded, do we really consider shipping.rates and .times to be an array ready for manipulation? For example, do we want to allow adding and removing items from it? For other entries like settings we have null not {}. I think we may do the same for rates and times

//cc @eason9487 @ecgan as you were supporting me in finding a solution for that.

[ecgan]

@tomalec , thanks for creating this issue.

The first problem we have is that the <Form> component uses initialValues to create its own internal state, then ignores subsequent changes made to this prop. I believe it's a faulty implementation and something that should be addressed there.

From the way I see it, it is behaving as expected, similar to many Form components out there (e.g. Formik, Ant Design Form Component, etc). If we want to update the form state with subsequent changes, we need to use the form's setter method (something like setValue).

Let me know if I'm missing something.

I also start to wonder whether [] is a good default/initial state. Before any data is loaded, do we really consider shipping.rates and .times to be an array ready for manipulation? For example, do we want to allow adding and removing items from it? For other entries like settings we have null not {}. I think we may do the same for rates and times

Yeah I don't think [] is a good default state, instead it should be null or undefined. I don't remember why I made it that way in the first place, maybe because I was still learning and tinkering with wp-data at that time. I think we shouldn't make the rates and times ready for manipulation before any data is loaded; we should always load the data first.

---

Talking about isResolving, since it is not a reliable way to indicate loading in progress, I'm having an idea that maybe we should keep track of our own loading state and error state (in addition to the API data) into our wp-data store, e.g.:

// current:
const { getShippingRates } = select( STORE_KEY ); 
const data = getShippingRates(); // this will give us the shipping rates data when API call is complete.

// new idea:
const { getShippingRates } = select( STORE_KEY ); 
const { loading, error, data } = getShippingRates(); // we can get access to loading, error, and data.

The above is pretty much similar to useApiFetchCallback and useApiFetchEffect.

Note that this may be a bad idea, I don't know for sure, but just want to put the idea out first.

[tomalec]

If we want to update the form state with subsequent changes, we need to use the form's setter method (something like setValue).

How can we do that from the parent component? (especially if we want to use declarative/functional approach).

What is the reason to block subsequent changes of initialValues?

---

Talking about isResolving, since it is not a reliable way to indicate loading in progress, I'm having an idea that maybe we should keep track of our own loading state and error state (in addition to the API data) into our wp-data store, e.g.:

👍

[ecgan]

How can we do that from the parent component? (especially if we want to use declarative/functional approach).

So instead of the following:

<Parent>
	<Form>
		<Child></Child>
	</Form>
</Parent>

We need to move the Form component up:

<Form>
	<Parent>
		<Child></Child>
	</Parent>
</Form>

Then the Parent component will have access to the Form's setter method.

What is the reason to block subsequent changes of initialValues?

Think of it from the Form component perspective: it needs to know an initial state, so that it is able to know what has been changed (dirtiness or pristineness of the form). The form state is used to display and keep track of the users' input, and therefore it needs to be shielded from other changes, we don't want other changes to overwrite users' input.

[tomalec]

Thanks for the explanations :) It seems we are collecting more and more reasons to change the structure of free listings (edit then perhaps eventually setup) and move the <Form> outside individual steps.

Then the Parent component will have access to the Form's setter method.

So, the answer to "How can we do that from the parent component?" is you don't, do that from the child component (<Parent> as a child of <Form>).
I think it highlights the switch in my mental model I need to make. From OO Web Components where parent manages the children to FP React where parents only create children, then children use methods and resources given by parents. "Grown-up kids" ;) parenting school for programmers 😆

---

Think of it from the Form component perspective: […]

Thanks, now it makes sense to me.

[ecgan]

@tomalec ,

I think it highlights the switch in my mental model I need to make. From OO Web Components where parent manages the children to FP React where parents only create children, then children use methods and resources given by parents.

Just wanted to make sure that you have the right switch in the mental model, here's a bit more explanation 😄 If you want the parent components to manage the values of your children, you can.

// Parent component controlling the value of child component.
const ParentAAA = () => {
	// initial value is empty string.
	const [ value, setValue ] = useState( '' );

	const handleChange = ( v ) => {
		setValue( v );
	};

	// child component's value is controlled by the parent.
	return <ChildBBB value={ value } onChange={ handleChange } />;
};

In the above, the ChildBBB's value is controlled by the parent via value={ value }. This concept is called controlled component. There's also uncontrolled component that works with the DOM.

From the above code example, imagine if the parent wants to render many children, then it would quickly become unmanageable to have many useState and handle***. This is where those form components come in to help.

// instead of using useState and event handlers manually, we use Form component to help us.
const ParentAAA = () => {
	return (
		<Form
			initialValues={{
				bbb: ''
			}}
		>
			{(formProps) => {
				const { getInputProps } = formProps

				return (
					<ChildBBB {...getInputProps('bbb')} />
				)
			}}
		</Form>
	)
};

Notice the similarity between the manual useState(initialValue) in the first example and the use of <Form initialValues={{}}> in the second example. When you see a component with an initial*** props (like the form component), it usually implies that it keeps its own state internally. ChildBBB is now controlled by the Form component, not ParentAAA.

I hope this explanation helps and gives you a better mental model 😄

[eason9487]

For a short-term solution for this issue scope, it could be addressed by some minor changes.

1. Change the initial values of mc.shipping.rates and mc.shipping.times to null
2. Replace isResolving by hasFinishedResolution and return an empty array instead of null in useShippingRates and useShippingTimes hooks

   const emptyList = [];
   
   const useShippingRates = () => {
   	return useSelect( ( select ) => {
   		const { getShippingRates, hasFinishedResolution } = select( STORE_KEY );
   
   		// Always returns an array to make it easier to use
   		// when the consuming side doesn't care whether the data loaded or not.
   		const data = getShippingRates() || emptyList;
   
   		// Use loading for check if the data is ready.
   		const loading = ! hasFinishedResolution( 'getShippingRates' );
   
   		return {
   			loading,
   			data,
   		};
   	} );
   };

3. Replace all getShippingRates and getShippingTimes selectors with corresponding hooks

[eason9487]

And for a long-term direction, I didn't find a standard practice, so I'd like to propose some ways to use wp.data based more on conventions. As I don't have enough time today to finish writing more detailed thoughts, I wrote the more critical ones in the form of comments on the relevant code, and I will add the rest later. 🙏

The proposal includes how to deal with status, data, and error with wp.data. Please refer to this commit 71bece8 to have a look.