Site Migration: Enable to manage when the site migration key will be loaded

[gabrielcaires]

Part of #89760

Proposed Changes

It is part of #89760 where we need to control a sequence of requests to ensure a site is ready to run the migrate-guru migration.

I am going to use this feature in further PRs.

• Introduce the enable prop on the use-site-migration-key hook to manage when we want to start fetching the key.
• Fix the typo on the file name

Testing Instructions

• Automated tests cover the change https://github.com/Automattic/wp-calypso/pull/90086/files#diff-775355931667ac238518fab522e40789af941e66df844fc570fc0f2f5bd2a918R39-R51
• The current site migration instructions is the only module using this hook, and it is also covered by automated tests as you can see here.

[github-actions]

Calypso Live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-105831

Jetpack Cloud live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-105831&env=jetpack

Automattic for Agencies live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-105831&env=a8c-for-agencies

[matticbot]

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

[andres-blanco]

I think it's confusing for the Options type to be UseQueryOptions. Looking at this hook from another programmer POV, I would expect to configure the whole query via this options parameter given it's type (like queryKey, queryFn, etc), but I can only affect retry and enabled in some special circumstances.

Since it looks like it will only be used to control retry & enabled I think a custom type for only those two things would be more clear.

[gabrielcaires]

@andres-blanco

I am not sure if I understood.

In this case, I'm just reusing the enabled type definition instead of being an "UseQueryOption type".

Are you suggesting something like:

interface Options {
 enabled: boolean?
}

Or are you suggesting open to receiving all useQueryOption attributes?

Using the Pick we can't pass all other attributes.
The idea is not to enable configuring the other parts because it is not generic and it can break the usage.

[andres-blanco]

Maybe I didn't explain myself correctly. Basically I don't see the value of picking from the UseQueryOption type since it is not going to affect the query directly, making the semantics slightly different.

Say you have the following code:
useSiteMigrationKey( null, { enabled: true } )

I would expect for the actual API call to be made, because I enabled it and it's a UseQueryOption pick, so it would be reasonable to override default behaviour. But it is not made because siteId is null.
If we had a custom type for this hook, say something like:

interface Options {
   stalling: boolean?
}

This purpose of this call useSiteMigrationKey( null, { stalling: true } ) would be clearer to me, because I wouldn't have an expectation about the inner implementation.

[gabrielcaires]

I gotcha, you won't to create false expectations by saying this enabled will not work exactly in a similar way to the original one.

I have mixed feelings, so I would like to think more.

Do you mind if I create a separate issue just to give me more time to think about it and not block it?

[andres-blanco]

I gotcha, you won't to create false expectations by saying this enabled will not work exactly in a similar way to the original one.

In my mind, the expectation is created by the enabled being picked from UseQueryOption not from it being called enabled.

Do you mind if I create a separate issue just to give me more time to think about it and not block it?

yup, go ahead! I'll approve all of this