DomainPicker: Move getFree/Paid/RecommendedDomainSuggestion helper to domain suggestions store.

(@yansern speaking here... lol)

UPDATE 28/05/2020: After wrangling with all the options I decided to go with none of them (but following closely to Option 4 as suggested by @razvanpapadopol).

Domain picker package will simply export getFree/Paid/Recommended/DomainSuggestion(s) as utility functions.

import {
    getFreeDomainSuggestions,
    getPaidDomainSuggestions,
    getRecommendedDomainSuggestion
} from '@automattic/domain-picker`;

More exploration thoughts in the comment here: https://github.com/Automattic/wp-calypso/issues/42567#issuecomment-635132030

The plan is to add getFreeDomainSuggestion(), getPaidDomainSuggestion(), getRecommendedSuggestion() selectors to to the domain-suggestions store.

Option 1: Accept `search` & `options` param for all selectors

The existing selector getDomainSuggestion(search, options) accepts two arguments: search and options.

If get[Free/Paid/Recommended]Suggestions selectors were to reuse the getDomainSuggestion selector, they would also need to accept the search and options parameters, which feels repetitive.

getDomainSuggestion(search, options)
getFreeDomainSuggestion(search, options)
getPaidDomainSuggestion(search, options)
getRecommendedDomainSuggestion(search, options)

Option 2: Keep `search` & `options` somewhere (reducer/action)

If search & options (or more specifically the normalizedQuery) are also props of the state, then all selectors can reuse them, hence you would have:

getDomainSuggestion()
getFreeDomainSuggestion()
getPaidDomainSuggestion()
getRecommendedDomainSuggestion()

And you would need to introduce two actions:

setSearchTerm()
setSearchOptions()

I wonder if the drawback to this method is that the data store would then always contains the current set of domain suggestions for the current search term and searhc options, and not allowing the flexibility of calling the selectors with different params that return different set of domain suggestions.

Option 3: `getDomainSuggestion` returns an object with keyed by categories.

Perhaps getDomainSuggestion() could instead return an object categorized by all, free, paid, recommended, e.g.

getDomainSuggestion(search, options)
// returns { all: [], free: [], paid: [], recommended: {} }

The weird part with this is that recommended actually returns a DomainSuggestion object not an array of DomainSuggestion[].

Options 4: Free/Paid/Recommmended should be handled by DomainPicker.

I think the source of a large proportion of the props flowing to domain-picker is the mixed logic happening in client/landing/gutenboarding/hooks/use-domain-suggestions.ts. I feel the fetching call in there should belong to domain-picker package and any processing done to allSuggestions should be handled directly from here. In this way, we can pass just an optional prop prioritisedSearch: string if we want to overwrite the search term.

Originally posted by @razvanpapadopol in https://github.com/Automattic/wp-calypso/pull/42409

I think I'm inclined to agree that free/paid/recommended processing being done on DomainPicker would reduce the amount of props passed into DomainPicker.

The argument to move getFree/Paid/RecommendedDomainSuggestion() to store was primarily because header/index.tsx is also using them. Header is using it to determine what domain should be used to display on the domain picker button.

Some additional thoughts but did not explore further.

get[Free/Paid/Recommended]DomainSuggestion(s) could actually belong in the existing data-stores/src/domain-suggestions/utils.ts file in data stores, since these functions deals with data manipulation.
Both Gutenboarding & Domain Picker component can import this utility function from data store. This convention of importing utility functions from data stores has never been done before, so it might feel a little weird. But one can argue that since importing types from data stores is already an existing practice, importing utility functions from data store might not be that weird after all.
The utility functions could instead be called filterByFreeDomainSuggestions(), filterByPaidDomainSuggestions() and findRecommendedDomainSuggestion(), as filter and find naming itself easily implies what it returns - array of items or a single item.
Most of the time we're getting the array of free domain suggestions just to use one of them, so we end up doing getFreeDomainSuggestions()?.[0], so getFreeDomainSuggestion() or findFreeDomainSuggestion() might be a nice sugar addition to the utility belt.
getCuratedDomainSuggestions() could be an interesting selector to introduce (based on Option 3 above) and it could reuse the utility function in the data stores. This will simplify how the code looks like in both header/index.tsx and domain-picker/index.tsx.
- One gotcha is that paidSuggestions can either be an array of 5 or 10 items. Naturally, this feels like a decision that should be made by the component. But this defeats the purpose of trying to simplify code by moving them to getCuratedDomainSuggestions() because we still have to do some data wrangling on the component end like curatedDomainSuggestions.paid.slice(0, 5).
- If some kind of param like paidLimit is introduced to the getCuratedDomainSuggestions() selector, it also feels like an oddball param.

The approach in #42738 doesn't seem that close to option 4. The domain picker component now exports some functions that are useful for transforming data selected from domains. That seems to lead to some very strange coupling 🤔

// Select from the store (`@automattic/data-stores` module. No direct import necessary)
const domains = select( 'automattic/domains/suggestions' ).getDomainSuggestions( 'my-query' );
// Transform domains data (`@automattic/domain-picker` module? This seems out of place)
const freeDomains = getFreeSuggestions( domains );
const paidDomains = getFreeSuggestions( domains );
const recommendedDomain = getRecommendedDomainSuggestion( domains );

The more I look at these, the more they look like selectors. I don't want to rush the wrong direction to remove some duplicated functionality. We can leave the duplicated functionality until we understand where it should go.

I see very little difference between a selector that returns grouped domains (free/paid/recommended…) in an object (option 3) and one selector per category:

function getGroupedDomainSuggestions(...args) {
  return {
    all: getDomainSuggestions( ...args ),
    free: getFreeDomainSuggestions( …args ),
    // snip…
  }
}
// returns { all: [], free: [], paid: [], recommended: {} }

I'm curious what the difficulties with moving to selectors on the store were. Can you elaborate? Maybe the useDomainSuggestions hook complicated things?

The approach in #42738 doesn't seem that close to option 4. The domain picker component now exports some functions that are useful for transforming data selected from domains. That seems to lead to some very strange coupling.

Ah I mean just the part where the logic is being done in the domain picker component instead.

The coupling is indeed strange. That's how it lead me a potentially silly idea as mentioned in https://github.com/Automattic/wp-calypso/issues/42567#issuecomment-635132030 that...if they're not selectors and remain as util functions, but placed in domain suggestions stores, but exported out for reuse. Which is also rather odd.

I'm curious what the difficulties with moving to selectors on the store were.

There are no difficulties in moving them as selectors, but rather the weirdness of having to pass in the same search & options parameters 4 times for each call.

const domainSuggestions = getDomainSuggestions( searchTerm, options );
const paidSuggestions = getFreeDomainSuggestions( searchTerm, options );
const freeSuggestions = getPaidDomainSuggestions( searchTerm, options );
const recommendedSuggestion = getRecommendedSuggestions( searchTerm, options );

Maybe I need to let that sink in and come to terms that it's actually not that weird.

Besides, the searchTerm & options gets serialized into a key and so there's actually no performance penalty in accessing the list of domains.

The other weirdness is that additional props would need to be introduced to the domain picker component, e.g.

<DomainPicker
     allSuggestions={}
     paidSuggestions={}
     freeSuggestions={}
     recommendedSuggestion={}>

Which leads the conversation of rather.. the <DomainPicker> component should accept a searchTerm & searchOptions and perform the search itself.

The more I look at these, the more they look like selectors. I don't want to rush the wrong direction to remove some duplicated functionality. We can leave the duplicated functionality until we understand where it should go.

I think I'm leaning towards the direction where the domain picker is going to accept searchTerm and searchOptions and that gets passed to the data stores where the API search is performed.

With selectors in the data stores, header/index.tsx can reuse them too.

Maybe the useDomainSuggestions hook complicated things?

Yes. At one point I was thinking maybe useDomainSuggestions() should return like an object like Option 3. Either that or introduce more hooks like usePaid/Free/RecommendedDomainSuggestions().

However, this also means <DomainPicker> needs to accept those additional props paidSuggestions/freeSuggestions/recommendedSuggestion, which takes us back full circle to the beginning of this conversation.

I think if we go with the direction where <DomainPicker> accepting searchTerm and searchOptions, then useDomainSuggestions hook might actually turn into a useDomainSearchOptions hook instead. (Need some exploration).

In conclusion, I think my vote is towards:

<DomainPicker> accepting searchTerm and searchOptions.
getFree/Paid/RecomendedDomainSuggestions to turn into selectors (Option 1).
useDomainSuggestions hook might get turned into useDomainSearchOptions() hook.

I like the direction you outline 👍

Yes. At one point I was thinking maybe useDomainSuggestions() should return like an object like Option 3. Either that or introduce more hooks like usePaid/Free/RecommendedDomainSuggestions().

Nice idea, maybe even returning the selector arguments:

const domainSelectParameters = useDomainSelectParameters();
const domainSuggestions = getDomainSuggestions( ...domainSelectParameters );
const paidSuggestions = getFreeDomainSuggestions( ...domainSelectParameters );
const freeSuggestions = getPaidDomainSuggestions( ...domainSelectParameters );
const recommendedSuggestion = getRecommendedSuggestion( ...domainSelectParameters );

Not sure if this can be fixed when refactoring the domain suggestions selectors but I think it's worth checking out: https://github.com/Automattic/wp-calypso/issues/42758

Fast forward. API has been simplified and the relevant bits have moved to the domain store or the component. Closing this issue.

Automattic / wp-calypso