Data: Create registry selector with self-contained registry proxying

[aduth]

Extracted from #16761

This pull request seeks to resolve an issue where calling a registry selector from a non-registry selector will produce an unexpected value, since the function returned by createRegistrySelector was the higher-order function expecting the registry as argument, not the underlying selector. With the changes here, a non-registry selector can call a registry selector as if it were any standard selector. The registry is automatically provided by the "current" registry in the mapping of selectors during store registration.

Testing Instructions:

Ensure unit tests pass:

npm run test-unit

[nerrad]

I think I grok why you are doing this, but I'm interested in knowing how the issue prompting this surfaced?

[nerrad]

It's unclear to me where selector.registry.select is coming from as the argument passed to registrySelector. Is it supposed to be defaultRegistry.select or I'm guessing it will it be whatever is set on selector.registry at the time it's invoked (with a fallback to defaultRegistry)?

[aduth]

It's unclear to me where selector.registry.select is coming from as the argument passed to registrySelector. Is it supposed to be defaultRegistry.select or I'm guessing it will it be whatever is set on selector.registry at the time it's invoked (with a fallback to defaultRegistry)?

It happens here:

gutenberg/packages/data/src/namespace-store/index.js

Lines 47 to 49 in 4edd1d0

	if ( selector.isRegistrySelector ) {
	selector.registry = registry;
	}

This occurs when a store is registered to a registry, so effectively the default registry is never used. I added it to simplify / respect the @type declaration (tangentially related to #16693), but it could just as well be a noop stub.

[nerrad]

Makes sense. I wonder if using a noop stub instead might lead to less head scratching for future looks at this code?

[aduth]

Makes sense. I wonder if using a noop stub instead might lead to less head scratching for future looks at this code?

If we stubbed it, I wouldn't feel comfortable adding @type {WPDataRegistry}, since it'd be wrong to claim that the stub is of that type (and I expect potential future type checking could flag it as such).

I'm leaning that the current comment for selector.registry assignment alone could be clear enough to explain its purpose.

https://github.com/WordPress/gutenberg/blob/a9e45dc3cdaa7696ac5e686a0ff6fe9eec1f9a5b/packages/data/src/factory.js#L25-L26

[aduth]

I think I grok why you are doing this, but I'm interested in knowing how the issue prompting this surfaced?

In a pending branch, I'm bulk-updating a number of selectors to soft-deprecate them in favor of equivalent selectors in another registry store. This impacts a number of non-updated selectors which simply derive data from what are now registry selectors.

Code example:

// packages/editor/src/store/selectors.js

export const getPostEdits = createRegistrySelector( ( select ) => ( state ) => {
	const postType = getCurrentPostType( state );
	const postId = getCurrentPostId( state );
	return select( 'core' ).getEntityRecordEdits( 'postType', postType, postId ) || EMPTY_OBJECT;
} );

export function hasChangedContent( state ) {
	const edits = getPostEdits( state );

	return '_blocks' in edits || 'content' in edits;
}

[aduth]

@nerrad @talldan I'd be curious to have your thoughts on how to address the failing tests here. It seems core/editor's isEditedPostAutosaveable is the only test where we try to stub registry selectors, and relied on the fact the function was returned in its higher-order function form (expecting to receive a registry):

gutenberg/packages/editor/src/store/test/selectors.js

Lines 1270 to 1280 in 4edd1d0

	const isEditedPostAutosaveable = isEditedPostAutosaveableRegistrySelector( () => ( {
	getCurrentUser() {},
	hasFetchedAutosaves() {
	return false;
	},
	getAutosave() {
	return {
	title: 'sassel',
	};
	},
	} ) );

One option might be to make the original, unwrapped selector available on some property (e.g. unwrappedSelector), but:

• I'm not sure it's something we want to be made available in the public interface, and it mostly benefits testing
• We should consider more what we think to be an ideal developer workflow for testing cross-store selectors in a registry. The code above relies on an assumption that we only call selectors on a single store core, but I doubt it will always be this simple.

Some related thoughts:

• We could assign a property depending on the process.env.NODE_ENV value to make it available only in testing / development environments
• Sinon-style calledWith could be a nice way to build out a stubbed select. There would be redundancy here to introduce Sinon when Jest already has its own mock functions, but they're not quite as easy to use for this purpose.

const isEditedPostAutosaveable = isEditedPostAutosaveable.unwrappedSelector(
	sinon.stub()
		.calledWith( 'core' ).returns( {
			getCurrentUser() {},
			// ...
		} )
);

[aduth]

I think I grok why you are doing this, but I'm interested in knowing how the issue prompting this surfaced?

In a pending branch, I'm bulk-updating a number of selectors to soft-deprecate them in favor of equivalent selectors in another registry store. [...]

The pull request for this is now available at #16761.

[nerrad]

I'd be curious to have your thoughts on how to address the failing tests here. It seems core/editor's isEditedPostAutosaveable is the only test where we try to stub registry selectors, and relied on the fact the function was returned in its higher-order function form (expecting to receive a registry):

Edit: I spoke too soon... I didn't test this on all the failing tests and there's some obvious flaws here :). I'll update when I've got something better to add. I now see the problem so all of the below is incorrect.

Click to see original suggestion which does _not_ work but left in case someone jumps to the same conclusion I did.

You're right, the issue here is that the function now returns a different value. However, it's still possible to mock the `registry.select` by just setting up the test differently (essentially reproducing what happens in the `@wordpress/data` module when the registry is constructed. I tried this locally in this branch and it worked.

it( 'should return false if existing autosaves have not yet been fetched', () => {
	const isEditedPostAutosaveable = isEditedPostAutosaveableRegistrySelector;
	isEditedPostAutosaveable.registry = () => ( {
		registry: {
			select: {
				getCurrentUser() {},
				hasFetchedAutosaves() {
					return false;
				},
				getAutosave() {
					return {
						title: 'sassel',
					};
				},
			},
		},
	} );

	const state = {
		editor: {
			present: {
				blocks: {
					value: [],
				},
				edits: {},
			},
		},
		initialEdits: {},
		currentPost: {
			title: 'sassel',
		},
		saving: {
			requesting: true,
		},
	};

	expect( isEditedPostAutosaveable( state ) ).toBe( false );
} );

Essentially these are the two key changes:

const isEditedPostAutosaveable = isEditedPostAutosaveableRegistrySelector;
isEditedPostAutosaveable.registry = () => ( { /** mocked registry object **/ } );

You probably could create a helper to set this up for each test condition.

[nerrad]

The only other thing I can think of here (other than what you already suggested @aduth) would be to mock createRegistrySelector itself so you can basically capture the incoming registrySelect value and enhance it for spying on the calls and returning simulated cross store selectors. I think that would be acceptable because createRegistrySelector behaviour is tested in @wordpress/data so it's internal logic is already covered. The tricky part here might be mocking createRegistrySelector though ;)

[talldan]

@aduth That's difficult to answer. The original implementation of those tests perhaps wasn't ideal.

It's a balance between testing as close to the actual implementation as possible, and making it nice and easy to write tests.

I tested creating a mock version of the core store, which seemed to work on one of the tests, and isn't much work. But it may require a tidy up between tests:

registerStore( 'core', {
	reducer: () => {},
	selectors: {
		getCurrentUser() {},
		hasFetchedAutosaves() {
			return false;
		},
		getAutosave() {
			return {
				title: 'sassel',
			};
		},
	},
} );

I'll have a think about it.

[aduth]

I spoke with @epiqueras about the approach for testing. He pushed up some changes in a9e45dc which I think are the simplest adaptation of the current approach which passes.

To the broader question of a pattern for testing registry selectors, @talldan I think your idea could work well, and perhaps we don't even need to register the store, but rather assign a faked registry on the selector instance property.

Example (via @epiqueras):

isEditedPostAutosaveableRegistrySelector.registry = {
    select() {
        return {
            getCurrentUser() {},
            hasFetchedAutosaves() {
                return false;
            },
            getAutosave() {
                return {
                    title: 'sassel',
                };
            },
        };
    },
};