Skip to content

Fix unwanted rich previews for internal urls #32613

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.

Sign up for GitHub

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

Closed
getdave wants to merge 4 commits into from
Closed

Fix unwanted rich previews for internal urls #32613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bc72aa1
Add baseURL to editor settings
getdave Jun 11, 2021
69ecf6b
Don't fetch rich data for internal URLS
getdave Jun 11, 2021
8813497
Don't show previews unless rich previews prop is truthy
getdave Jun 11, 2021
26d90d9
Use base URL from server side settings
getdave Jun 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/editor-settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ function gutenberg_extend_post_editor_settings( $settings ) {

$settings['imageDefaultSize'] = in_array( $image_default_size, $image_sizes, true ) ? $image_default_size : 'large';
$settings['__unstableEnableFullSiteEditingBlocks'] = gutenberg_supports_block_templates();
$settings['__experimentalBaseUrl'] = get_site_url();

if ( gutenberg_is_fse_theme() ) {
$settings['defaultTemplatePartAreas'] = gutenberg_get_allowed_template_part_areas();
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/link-control/link-preview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export default function LinkPreview( {
<ViewerSlot fillProps={ value } />
</div>

{ ( hasRichData || isFetching ) && (
{ hasRichPreviews && ( hasRichData || isFetching ) && (
<div className="block-editor-link-control__search-item-bottom">
<div
aria-hidden={ ! richData?.image }
Expand Down
10 changes: 9 additions & 1 deletion packages/block-editor/src/components/link-control/use-remote-url-data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,27 @@ function useRemoteUrlData( url ) {
isFetching: false,
} );

const { fetchRemoteUrlData } = useSelect( ( select ) => {
const { fetchRemoteUrlData, baseUrl } = useSelect( ( select ) => {
const { getSettings } = select( blockEditorStore );
return {
baseUrl: getSettings().__experimentalBaseUrl,
fetchRemoteUrlData: getSettings().__experimentalFetchRemoteUrlData,
Copy link
Contributor

@youknowriad youknowriad Jun 11, 2021

Choose a reason for hiding this comment

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

So If I'm not wrong, this feature requires two block-editor settings, It feels a bit too much for just previews tbh. Can't we introduce the bail out behavior inside __experimentalFetchRemoteUrlData somehow?

Copy link
Contributor Author

@getdave getdave Jun 11, 2021

Choose a reason for hiding this comment

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

That was my first thought. Should that need to know about the concept of an internal URL though?

I can look into that later on today.

Copy link
Contributor

@youknowriad youknowriad Jun 11, 2021

Choose a reason for hiding this comment

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

Maybe the block-editor doesn't need to know about "fetchRemoteURL" entirely? I'm just thinking out loud but maybe the setting should be renderURLPreview (render function)

Copy link
Contributor Author

@getdave getdave Jun 11, 2021

Choose a reason for hiding this comment

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

That is an interesting option.

Copy link
Member

@kevin940726 kevin940726 Jun 12, 2021

Choose a reason for hiding this comment

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

But does it mean that the users have to implement the loading and error status/style by themselves?

How about instead of accepting an async function (fetchRemoteUrlData), or a render function (renderUrlPreview), we simply accept a React hook (useRemoteUrlData) as the setting value? The hook takes a remote url as the argument, and return richData and isFetching, just like the one we've already created.

useRemoteUrlData( url ) {
  // ...

  return { richData, isFetching };
}

This way, the users don't have to implement the loading style, and they can also skip fetching internal URLs if the url matches a certain pattern. There are no intermediate loading status like we have in async function since that everything can return synchronously. In the future, we can also handle errors or support other progressive enhancements directly in the hook.

Copy link
Contributor

@youknowriad youknowriad Jun 14, 2021

Choose a reason for hiding this comment

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

so it would be something like:

const { useRemoteURLData } = useSelect( select => select( 'block-editor' ).getSettings(), [] );
const { richData, isLoading } = useRemoteURLData();

this kind of breaks the rules of hooks, because that hook is not necessarily defined and can be updated over time to call different built-in hooks.

I do think it could be nice, but maybe a bit fragile and a bit weird as well (a react hook as a setting).

WDYT

Copy link
Contributor Author

@getdave getdave Jun 14, 2021

Choose a reason for hiding this comment

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

@youknowriad What you have is (I think) how @kevin940726 and I imagined it.

because that hook is not necessarily defined

That's probably the biggest problem for me. It's likely to only be defined in the Editor and not the Block Editor.

Copy link
Contributor Author

@getdave getdave Jun 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

How about we change the setting to fetchRichData and remove the current __experimentalFetchRemoteUrlData.

This will accept a url much as __experimentalFetchRemoteUrlData does now. But within the function, we can decide which path to follow based on whether the url is internal or not.

Initially the implementation would be something like:

function __experimentalFetchRichData(url) {
    if (url.includes(baseUrl)) {
        return Promise.resolve({})'
    }

    // Now do the same as `__experimentalFetchRemoteUrlData` does currently
    // to handle external URL
}

Then in the future we could bolt on the ability to fetch rich data about an internal URL very easily.

This allows us to keep the current hook implementation the same whilst also making sure that block-editor has no idea about WP specific stuff.

Copy link
Contributor Author

@getdave getdave Jun 14, 2021

Choose a reason for hiding this comment

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

See #32658

Copy link
Member

@kevin940726 kevin940726 Jun 15, 2021

Choose a reason for hiding this comment

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

What I was thinking is to split <LinkPreview> into 2 different components, one is the original LinkPreview, and the other one is the LinkRichPreview component. We can skip rendering <LinkRichPreview> if there's no useRemoteURLData (or any other name) so that we don't break the rules of hooks. And I would imagine the setting can't be updated once it's rendered so that it won't break the other rule of hooks either.

But I agree that it's a bit weird though. Maybe render function (or just a component) is a better way to go, but would need to update LinkPreview to make it work.

};
}, [] );

// We only want to fetch info for remote (external) URLS.
// URL is considered internal if:
// 1. We haven't got a base URL (yet).
// 2. The URL is being requested includes the base URL.
const isInternal = !! ( baseUrl && url?.includes( baseUrl ) );

useEffect( () => {
// Only make the request if we have an actual URL
// and the fetching util is available. In some editors
// there may not be such a util.
if (
url?.length &&
! isInternal &&
fetchRemoteUrlData &&
typeof AbortController !== 'undefined'
) {
Expand Down
1 change: 1 addition & 0 deletions packages/editor/src/components/provider/use-block-editor-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ function useBlockEditorSettings( settings, hasTemplate ) {
'__experimentalGlobalStylesUserEntityId',
'__experimentalPreferredStyleVariations',
'__experimentalSetIsInserterOpened',
'__experimentalBaseUrl',
'alignWide',
'allowedBlockTypes',
'bodyPlaceholder',
Expand Down