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

[RNMobile] Add WP hook for registering non-core blocks #52791

Merged
merged 3 commits into from
Jul 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 0 additions & 5 deletions packages/react-native-editor/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,6 @@ const registerGutenberg = ( {
);
}

componentDidMount() {
// Dispatch post-render hooks.
doAction( 'native.render', this.filteredProps );
}

render() {
return cloneElement( this.editorComponent, this.filteredProps );
}
Expand Down
4 changes: 3 additions & 1 deletion packages/react-native-editor/src/setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { I18nManager, LogBox } from 'react-native';
* WordPress dependencies
*/
import { unregisterBlockType, getBlockType } from '@wordpress/blocks';
import { addAction, addFilter } from '@wordpress/hooks';
import { addAction, addFilter, doAction } from '@wordpress/hooks';
import * as wpData from '@wordpress/data';
import { registerCoreBlocks } from '@wordpress/block-library';
// eslint-disable-next-line no-restricted-imports
Expand Down Expand Up @@ -70,6 +70,8 @@ const setupInitHooks = () => {
) {
unregisterBlockType( 'core/block' );
}

doAction( 'native.post-register-core-blocks', props );
Copy link
Member

Choose a reason for hiding this comment

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

I've double-checked that the props passed here are the ones expected as from the older approach in https://github.com/WordPress/gutenberg/blob/trunk/packages/react-native-editor/src/index.js#L57-L66 👍

I was wondering if we should remove the native.render action or keep it if it's needed in the future, what do you think? I'm fine with keeping it by the way.

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've double-checked that the props passed here are the ones expected as from the older approach in https://github.com/WordPress/gutenberg/blob/trunk/packages/react-native-editor/src/index.js#L57-L66 👍

Thanks for checking it 🙇 ! I saw that we only use the capabilities prop when registering the Jetpack blocks, which is already being passed.

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 was wondering if we should remove the native.render action or keep it if it's needed in the future, what do you think? I'm fine with keeping it by the way.

Good question. AFAIK it's not being used anywhere, so I understand we could remove it. If we need it in the future we can always revert it back.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me! Are we planning to do that in a follow-up PR? This PR is ready for my approval so let me know!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry. I'm planning to apply the suggestion in this PR. In fact, I noticed that I missed adding a unit test for the new hook. I'll let you know when I pushed the changes, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Cool! I'll keep an eye out for those, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@geriux I've applied you suggestion 2edd4a8.

I also updated the unit tests in 7fb8ba6. It took way longer than I expected because the test logic required some tricky changes to check the call orders. Let me know if you have any concerns/questions regarding that, thanks 🙇 !

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I'll check it out

} );

// Map native props to Editor props
Expand Down
62 changes: 38 additions & 24 deletions packages/react-native-editor/src/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ import { initializeEditor, render } from 'test/helpers';
* WordPress dependencies
*/
import * as wpHooks from '@wordpress/hooks';
import { registerCoreBlocks } from '@wordpress/block-library';
// eslint-disable-next-line no-restricted-imports
import * as wpEditPost from '@wordpress/edit-post';
import '@wordpress/jest-console';

/**
Expand All @@ -18,6 +21,11 @@ import setupLocale from '../setup-locale';

jest.mock( 'react-native/Libraries/ReactNative/AppRegistry' );
jest.mock( '../setup-locale' );
jest.mock( '@wordpress/block-library', () => ( {
__esModule: true,
registerCoreBlocks: jest.fn(),
NEW_BLOCK_TYPES: {},
} ) );

const getEditorComponent = ( registerParams ) => {
let EditorComponent;
Expand Down Expand Up @@ -150,50 +158,56 @@ describe( 'Register Gutenberg', () => {
expect( hookCallOrder ).toBeLessThan( onRenderEditorCallOrder );
} );

it( 'dispatches "native.render" hook after the editor is rendered', () => {
const doAction = jest.spyOn( wpHooks, 'doAction' );

it( 'dispatches "native.post-register-core-blocks" hook after core blocks are registered', async () => {
// An empty component is provided in order to listen for render calls of the editor component.
const onRenderEditor = jest.fn();
const MockEditor = () => {
onRenderEditor();
return null;
};
jest.mock( '../setup', () => ( {
__esModule: true,
default: jest.fn().mockReturnValue( <MockEditor /> ),
} ) );

// Unmock setup module to render the above mocked editor component.
jest.unmock( '../setup' );

// The mocked editor component is provided via `initializeEditor` function of
// `@wordpress/edit-post` package, instead of via the setup as above test cases.
const initializeEditorMock = jest
.spyOn( wpEditPost, 'initializeEditor' )
.mockReturnValue( <MockEditor /> );

// Listen to WP hook
const callback = jest.fn();
wpHooks.addAction(
'native.post-register-core-blocks',
'test',
callback
);

const EditorComponent = getEditorComponent();
// Modules are isolated upon editor rendering in order to guarantee that the setup module is imported on every test.
jest.isolateModules( () => render( <EditorComponent /> ) );
render( <EditorComponent /> );

const hookCallIndex = 1;
// "invocationCallOrder" can be used to compare call orders between different mocks.
// Reference: https://github.com/facebook/jest/issues/4402#issuecomment-534516219
const hookCallOrder =
doAction.mock.invocationCallOrder[ hookCallIndex ];
const callbackCallOrder = callback.mock.invocationCallOrder[ 0 ];
const registerCoreBlocksCallOrder =
registerCoreBlocks.mock.invocationCallOrder[ 0 ];
const onRenderEditorCallOrder =
onRenderEditor.mock.invocationCallOrder[ 0 ];
const hookName = doAction.mock.calls[ hookCallIndex ][ 0 ];

expect( hookName ).toBe( 'native.render' );
expect( hookCallOrder ).toBeGreaterThan( onRenderEditorCallOrder );
expect( callbackCallOrder ).toBeGreaterThan(
registerCoreBlocksCallOrder
);
expect( callbackCallOrder ).toBeLessThan( onRenderEditorCallOrder );
expect( console ).toHaveLoggedWith( 'Hermes is: true' );

initializeEditorMock.mockRestore();
} );

it( 'initializes the editor', async () => {
// Unmock setup module to render the actual editor component.
jest.unmock( '../setup' );

const EditorComponent = getEditorComponent();
const screen = await initializeEditor(
{},
{ component: EditorComponent }
);
const screen = await initializeEditor();
// Inner blocks create BlockLists so let's take into account selecting the main one
const blockList = screen.getAllByTestId( 'block-list-wrapper' )[ 0 ];

expect( blockList ).toBeVisible();
expect( console ).toHaveLoggedWith( 'Hermes is: true' );
} );
} );