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

Site Editor: Improve loading experience (v3) #50222

Merged
merged 10 commits into from
May 10, 2023
15 changes: 15 additions & 0 deletions packages/data/src/redux-store/metadata/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,3 +131,18 @@ export function isResolving( state, selectorName, args ) {
export function getCachedResolvers( state ) {
return state;
}

/**
* Whether the store has any currently resolving selectors.
*
* @param {State} state Data state.
*
* @return {boolean} True if one or more selectors are resolving, false otherwise.
*/
export function hasResolvingSelectors( state ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending how confident we are with this selector (it seems fine to me), we could consider making it private to start with.

Copy link
Member Author

Choose a reason for hiding this comment

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

To me, one of the positive sides of metadata selectors is that they are always publicly available. That being said, I wonder if you think there's a particular need for this specific one to be private.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not for me. I think it's fine for this one to be public. I was suggesting it in case we missed confidence.

return [ ...Object.values( state ) ].some( ( selectorState ) =>
[ ...selectorState._map.values() ].some(
Comment on lines +143 to +144
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Are the copies necessary? [ ...Object.values( state ) ].some -> Object.values( state ).some, and likewise for selectorState.

  2. After digging around a bit, it seems like ._map is an internal property of package equivalent-key-map. Right? My follow-up questions are: 1) is this the right/only way to consume it? 2) If so, shouldn't we at least have a comment explaining how and why we are using a private interface of an external package?

Copy link
Member

Choose a reason for hiding this comment

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

The spread operators would be needed for results of Map.prototype.values(), because that's an iterator. But Object.values() returns an array, and that doesn't need the extra [...]. So, one of the spreads is justified, the other is not.

The .map.values() call indeed depends too much on the implementation details, and it seems to me it works only when the selector has exactly one argument? Because then the value is not in resolution[ 1 ], but deeper in the tree. But the only remedy is to contribute a new values() method to equivalent-key-map itself.

Copy link
Member Author

@tyxla tyxla May 23, 2023

Choose a reason for hiding this comment

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

Thank you for the feedback @mcsf! I've addressed it in #50865. A couple of notes to add:

  1. The first copy was indeed unnecessary, but the second one is necessary since the returned iterable iterator object doesn't have a .some(), thus we need to convert it to an array. We could use the EquivalentKeyMap's forEach() method, however, this would be less optimized since it often will require more iterations than a .some().

  2. I've added a comment to explain that usage. As mentioned above, .some() is a more optimized usage since it doesn't require iterating through all elements, which is why I prefer it to the .forEach() that EquivalentKeyMap supports.

Alternatively, we could open a PR in the EquivalentKeyMap repo for supporting .some() or a method to just retrieve all elements from the map if you think that will help.

Edit: ha, Jarda beat me to it. Spot on btw!

Copy link
Member

Choose a reason for hiding this comment

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

Note that the standard Map doesn't support .some. Only .keys, .values, and .entries, and unlike the similar Object.* methods they return iterators. Only after converting to array with [...] or Array.from you can call .every or .some.

What we'd like to do is to add .keys, .values and .entries to EKM. And I also suspect that their (or, more precisely, Andrew's 🙂) .size implementation is wrong, because it returns ._map.size, but the ._map is a tree with potentially many nested leaf nodes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the answers, both of you. Right, I didn't take the time to understand the data structure behind selectorState, and indeed .values in the context of a map should just be an iterator.

As mentioned above, .some() is a more optimized usage since it doesn't require iterating through all elements, which is why I prefer it to the .forEach() that EquivalentKeyMap supports.

To be clear, I think .some is the best idiom for the situation, and I wouldn't want to replace it with .forEach.

I'm fine with just removing the copy around Object.values and leaving the other as is. However, just a minor question: personally and intuitively, do you make a difference between [ ...xs ] and Array.from( xs )? For me, I tend to infer that the purpose of the former is to copy, while that of the latter is to cast (from iterator to array). If I'm the only one with this intuition, then ignore me; if not, then maybe this piece of code becomes 0.1% clearer by using Array.from( selectorState._map.values ).some. 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH spread and Array.from are all the same for me in the current use case (they do have some differences, where Array.from() is a bit more powerful and supports more use cases than iterables, but here it doesn't really matter.

Regardless, I'm happy to make this 0.1% more readable 👍 Updated in the PR.

Copy link
Member

Choose a reason for hiding this comment

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

I usually prefer Array.from, because it does the same thing, but is less obscure: it's a regular function call, not a special syntax. Spread operator is more useful when spreading multiple things together.

( resolution ) => resolution[ 1 ]?.status === 'resolving'
)
);
}
32 changes: 32 additions & 0 deletions packages/data/src/redux-store/metadata/test/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -324,3 +324,35 @@ describe( 'getResolutionError', () => {
).toBeFalsy();
} );
} );

describe( 'hasResolvingSelectors', () => {
let registry;
beforeEach( () => {
registry = createRegistry();
registry.registerStore( 'testStore', testStore );
} );

it( 'returns false if no requests have started', () => {
const { hasResolvingSelectors } = registry.select( 'testStore' );
const result = hasResolvingSelectors();

expect( result ).toBe( false );
} );

it( 'returns false if all requests have finished', () => {
registry.dispatch( 'testStore' ).startResolution( 'getFoo', [] );
registry.dispatch( 'testStore' ).finishResolution( 'getFoo', [] );
const { hasResolvingSelectors } = registry.select( 'testStore' );
const result = hasResolvingSelectors();

expect( result ).toBe( false );
} );

it( 'returns true if has started but not finished', () => {
registry.dispatch( 'testStore' ).startResolution( 'getFoo', [] );
const { hasResolvingSelectors } = registry.select( 'testStore' );
const result = hasResolvingSelectors();

expect( result ).toBe( true );
} );
} );
1 change: 1 addition & 0 deletions packages/data/src/redux-store/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@ describe( 'resolveSelect', () => {

it( 'returns only store native selectors and excludes all meta ones', () => {
expect( Object.keys( registry.resolveSelect( 'store' ) ) ).toEqual( [
'hasResolvingSelectors',
'getItems',
'getItemsNoResolver',
] );
Expand Down
2 changes: 2 additions & 0 deletions packages/e2e-test-utils/src/site-editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { addQueryArgs } from '@wordpress/url';

const SELECTORS = {
visualEditor: '.edit-site-visual-editor iframe',
loadingSpinner: '.edit-site-canvas-spinner',
};

/**
Expand Down Expand Up @@ -128,6 +129,7 @@ export async function visitSiteEditor( query, skipWelcomeGuide = true ) {

await visitAdminPage( 'site-editor.php', query );
await page.waitForSelector( SELECTORS.visualEditor );
await page.waitForSelector( SELECTORS.loadingSpinner, { hidden: true } );

if ( skipWelcomeGuide ) {
await disableSiteEditorWelcomeGuide();
Expand Down
39 changes: 34 additions & 5 deletions packages/edit-site/src/components/editor/index.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
/**
* WordPress dependencies
*/
import { useMemo } from '@wordpress/element';
import { useEffect, useMemo, useRef, useState } from '@wordpress/element';
import { useSelect, useDispatch } from '@wordpress/data';
import { Notice } from '@wordpress/components';
import { EntityProvider } from '@wordpress/core-data';
import { EntityProvider, store as coreStore } from '@wordpress/core-data';
import { store as preferencesStore } from '@wordpress/preferences';
import {
BlockContextProvider,
Expand Down Expand Up @@ -153,12 +153,41 @@ export default function Editor() {
// action in <URlQueryController> from double-announcing.
useTitle( hasLoadedPost && title );

if ( ! hasLoadedPost ) {
return <CanvasSpinner />;
}
const { hasResolvingSelectors } = useSelect( ( select ) => {
return {
hasResolvingSelectors: select( coreStore ).hasResolvingSelectors(),
};
} );
const [ loaded, setLoaded ] = useState( false );
const timeoutRef = useRef( null );

useEffect( () => {
if ( ! hasResolvingSelectors && ! loaded ) {
clearTimeout( timeoutRef.current );

/*
* We're using an arbitrary 1s timeout here to catch brief moments
* without any resolving selectors that would result in displaying
* brief flickers of loading state and loaded state.
*
* It's worth experimenting with different values, since this also
* adds 1s of artificial delay after loading has finished.
*/
timeoutRef.current = setTimeout( () => {
setLoaded( true );
}, 1000 );
Copy link
Member

Choose a reason for hiding this comment

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

Curious to know what this arbitrary timeout is for?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are brief moments where there aren't any resolving selectors, but a few milliseconds later, there are resolving selectors present. This arbitrary timeout is there to ensure that we're marking the editor as loaded only if there haven't been any resolving selectors for an entire second.

Copy link
Member

Choose a reason for hiding this comment

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

This reads a bit like an anti-pattern to me and a bad UX for users. Now every user has to wait at least a second to load the editor, no matter how fast their device is. Could there also be cases when the user's device is so slow that the 1-second delay isn't sufficient to run all the remaining tasks?

Could we figure out the correct timing to subscribe to the load event under the hood? This might be a bit more difficult to do but I think it'll be worth it 😆. At the very least, let's add some comments to explain the timeout.

Copy link
Contributor

@talldan talldan May 4, 2023

Choose a reason for hiding this comment

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

In terms of blocks, it seems like it's difficult to know what's going to load next. The next resource/block to load is revealed only once the previous resource/block has been loaded, parsed and rendered. My assumption is that's what this timeout is for.

The two ideas I had:

  • Be able to parse block content to determine what resources will load next. This would require a consistent way to declare resources for blocks. Only hide the loading spinner when there are no more known resources and all resolution has finished.
  • Somehow wait until all known inner blocks have rendered (and assume those blocks will have made their next requests and aren't waiting to do so).

Copy link
Member Author

Choose a reason for hiding this comment

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

This reads a bit like an anti-pattern to me and a bad UX for users. Now every user has to wait at least a second to load the editor, no matter how fast their device is. Could there also be cases when the user's device is so slow that the 1-second delay isn't sufficient to run all the remaining tasks?

I don't claim for the 1s to be perfect. I claim it to be much better than what we had before, and much better than if we didn't have it all (in which case we'd be seeing bad flickering).

Yes, 1s might be insufficient for very slow devices, but we're making a compromise here and I'm happy to experiment with timing separately. Regardless, I think this is much better than what we had before, so it's worth going with it for the time being while experimenting with better timing.

Could we figure out the correct timing to subscribe to the load event under the hood? This might be a bit more difficult to do but I think it'll be worth it 😆. At the very least, let's add some comments to explain the timeout.

I'm open to suggestions. From my understanding, there isn't a good way to predict or subscribe to that, nor to figure out the perfect timing without a lot of experimentation and speculation. @talldan has explained that well in the above comment.

In the meantime, I've added a comment to explain the arbitrary timeout: c7fed44

The two ideas I had:

Both of those seem pretty complex to me, and I'm not sure it's worth investing in an additional complex parsing mechanism or a complex loading tracking mechanism, given that we're aiming to start async loading blocks at some point in the near future (with the upcoming work on ES modules and import maps adoption in the WordPress core).

Copy link
Contributor

@talldan talldan May 10, 2023

Choose a reason for hiding this comment

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

Agree with all your points @tyxla 👍

Great job on getting this merged. 🎉


return () => {
clearTimeout( timeoutRef.current );
};
}
}, [ loaded, hasResolvingSelectors ] );

const isLoading = ! loaded || ! hasLoadedPost;
Copy link
Contributor

Choose a reason for hiding this comment

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

Potentially this logic to compute the isLoading const could be extracted to a custom hook useSiteEditorLoading or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

While I don't see the immediate need for that, I'm happy to go ahead and extract it when it can actually be reused somewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

it is mostly a suggestion to keep that logic outside the Editor component (timeout...) But I don't feel strongly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is a good idea, I'll follow up in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in #50511


return (
<>
{ isLoading ? <CanvasSpinner /> : null }
{ isEditMode && <WelcomeGuide /> }
<EntityProvider kind="root" type="site">
<EntityProvider
Expand Down
1 change: 1 addition & 0 deletions packages/edit-site/src/components/layout/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@
left: 0;
bottom: 0;
width: 100%;
overflow: hidden;

& > div {
color: $gray-900;
Expand Down