Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try migrate link suggestions to use Core Data #57582

Draft
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Jan 5, 2024

What?

Implements a selector for fetching link suggestions and utilises in <LinkControl>.

Why?

Currently LinkControl calls the fetchLinkSuggestions function directly. This means we don't get the benefits of caching and in Core Data so for each editor session the data is refetched for identical queries.

How?

Implements a Core Data selector which proxies to the original fetchLinkSuggstions under the hood. The benefit is now we get caching for free.

Alternatives Considered

Alternative approaches might be:

  • don't do this at all - it might be fine as it is 🤷‍♂️
  • implement a dedicated Block Editor REST API endpoint for "Link Suggestions". Then implement related Core Data selectors. This will avoid the need for multiple queries to the "search" endpoint and allow it to be customisable by extenders.

Testing Instructions

  • Open devtools Network tab and filter by XHR and search.
  • New Post
  • Type text and select a portion of it
  • CMD + K to make a link
  • Now type Sample. You should see search results shown after a short loading delay in UI and network tab.
  • Clear the search input.
  • Type Sample again. This time there should be no loading delay and results should show instantly. Also no additional network requests should be dispatched.

Testing Instructions for Keyboard

Screenshots or screencast

@getdave getdave self-assigned this Jan 5, 2024
@getdave getdave added [Package] Core data /packages/core-data [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) labels Jan 5, 2024
Copy link

github-actions bot commented Jan 5, 2024

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: [Package] Core data, [Feature] Link Editing.

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

Comment on lines +377 to +391
### getLinkSuggestions

Fetch link suggestions for a given search term.

_Parameters_

- _state_ Data state.
- _search_ Search term.
- _searchOptions_
- _settings_

_Returns_

- Link suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make this private. But I wasn't sure how to unlock a resolveSelect.

* @param settings
* @return Link suggestions.
*/
export function getLinkSuggestions(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a private API.

@@ -215,6 +214,10 @@ function useBlockEditorSettings( settings, postType, postId ) {
[ saveEntityRecord, userCanCreatePages ]
);

const syncGetLinkSuggestions = useCallback( async ( ...args ) => {
return await resolveSelect( coreStore ).getLinkSuggestions( ...args );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make this private how do we unlock a resolveSelect?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use a normal select call in a useSelect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we did this then we'd effectively pass something like getEntityRecords as a setting. We can't pass the result of a selector call because we need different args per invocation.

I think I tried doing this and found it didn't work. I could be wrong - would you expect it to?

case 'RECEIVE_LINK_SUGGESTIONS':
return {
...state,
[ action.search ]: action.suggestions,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this indexing strategy is good enough. A search is a simple string but the API allows for searching by different entities such as post, page...etc. So if we just index by the search you could accidentally receives results for the wrong content type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) [Package] Core data /packages/core-data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants