Block Editor: Flatten LinkControl components by mocking useSelect for tests

[aduth]

Previously: https://github.com/WordPress/gutenberg/pull/19638/files#r366440796

This pull request seeks to flatten the LinkControl component to remove the wrapper component added in #19638 which had been used for the purpose of preserving test behavior. The changes here resolve this by providing a mock for the useSelect function in the tests.

Testing Instructions:

Ensure unit tests pass:

npm run test-unit packages/block-editor/src/components/link-control/index.js

[youknowriad]

👍

[youknowriad]

I'm pretty sure we do requireActual( "@wordpress/*" ) for other packages. I wonder if it's broken there as well or if it works only for some packages.

[aduth]

I'm pretty sure we do requireActual( "@wordpress/*" ) for other packages. I wonder if it's broken there as well or if it works only for some packages.

Yeah, I didn't actually look in to it too much, but it was resolving to something, it just seemed it have been not the intended import file (from npm vs. from working directory? src vs. build vs. build-module?). I/we should probably do more to understand what's actually happening.

[youknowriad]

Thanks for fixing that.

[aduth]

Interesting build failure:

FAIL packages/block-editor/src/components/link-control/test/index.js
  ● Test suite failed to run
    Cannot find module '../../../../../data' from 'index.js'

I don't see it locally. But clearly related to the changes here.

I'll dig into it.

[aduth]

I don't see it locally. But clearly related to the changes here.

Ah, I think as written, the tests assume to resolve from the built file, but we don't require that these exist for running the tests (the tests do their own transpilation via babel-jest). Probably relates to what I mentioned at #19705 (comment).

I'll see what can be done about getting it to work with resolving from source like it should be expecting to.

[aduth]

I expect d66a4ce should pass. I'm not entirely happy with it as a solution, but there's definitely something strange happening when trying to mock the top-level module. I think it may have something to do with the specific way we export modules from this file (export ... from syntax?).

Jest reports a different result of require.requireActual( '@wordpress/data' ) depending on if it's inside or outside of a jest.mock callback, which is both unexpected but gives some insight into what might be going wrong (with undefined values being in the result when in a mock callback):

Inside jest.mock

Outside jest.mock

console.log node_modules/jest-mock/build/index.js:860 { combineReducers: [Getter], withSelect: [Getter], withDispatch: [Getter], withRegistry: [Getter], RegistryProvider: [Getter], RegistryConsumer: [Getter], useRegistry: [Getter], useSelect: [Getter], useDispatch: [Getter], __unstableUseDispatchWithMap: [Getter], AsyncModeProvider: [Getter], createRegistry: [Getter], createRegistrySelector: [Getter], createRegistryControl: [Getter], select: undefined, __experimentalResolveSelect: undefined, dispatch: undefined, subscribe: undefined, registerGenericStore: undefined, registerStore: undefined, use: undefined, plugins: undefined }

console.log node_modules/jest-mock/build/index.js:860 { combineReducers: [Getter], withSelect: [Getter], withDispatch: [Getter], withRegistry: [Getter], RegistryProvider: [Getter], RegistryConsumer: [Getter], useRegistry: [Getter], useSelect: [Getter], useDispatch: [Getter], __unstableUseDispatchWithMap: [Getter], AsyncModeProvider: [Getter], createRegistry: [Getter], createRegistrySelector: [Getter], createRegistryControl: [Getter], select: [Function], __experimentalResolveSelect: [Function], dispatch: [Function], subscribe: [Function], registerGenericStore: [Function], registerStore: [Function], use: [Function], plugins: { controls: [Getter], persistence: [Getter] } }

I also tried a few suggestions from jestjs/jest#936 and the documentation for require.requireActual (which includes some mention of using __esModule) without any success.

[aduth]

@youknowriad Am I correct in thinking that I'd read a comment from you that fetchSearchSuggestions was intentionally removed as a public prop of the component? I thought based on the changes here, we'd not be making it public:

https://github.com/WordPress/gutenberg/pull/19638/files#diff-106913d5079b57c6032c0775292d8d9dL240

And as such, I was going to remove the reference to it in the documentation:

https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/link-control/README.md#fetchsearchsuggestions

But now that I look more closely, I see that in the previous implementation, a custom fetchSearchSuggestions prop could still have been passed to override the default from the useSelect result, based on how the props are spread:

https://github.com/WordPress/gutenberg/pull/19638/files#diff-106913d5079b57c6032c0775292d8d9dR221

With the changes in this pull request, it's no longer received as a prop.

While all the tests passed, I'll want to double-check that we're not using it anywhere, in case I might have caused a regression. But do you think it's a prop we'd want anyways?

[aduth]

Follow-up at #19710

[youknowriad]

@aduth correct, I removed it explicitly. My thinking was that for the "block-editor" version of LinkControl, there's no need for this prop and I believe it was there essentially for tests.

If we decide later to have a UI version of LinkControl in "components" package, we could bring it back.