Sign in with Google block causes E2E test failure due to console warning

[tofumatt]

Bug Description

The Sign in with Google block is erroneously loaded in an admin-bar.test.js E2E test:

site-kit-wp/tests/e2e/specs/modules/search-console/admin-bar.test.js

Line 119 in 9d931c9

it( 'loads when editing a post with data in Search Console', async () => {

When it's loaded, it outputs a console warning that does not appear in production:

Steps to reproduce

1. Build test files locally npm run build:test
2. Run npm run test:e2e -- tests/e2e/specs/modules/search-console/admin-bar.test.js
3. Observe failed test

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• E2E tests should not fail due to Sign in with Google block code.

Implementation Brief

This fix has been verified in this PR

• Update includes/Modules/Sign_In_With_Google.php to only add the blocks-sign-in-with-google script (and styles) if the module is connected by adding a $this->connected() check in the following if statement. Also add a check for Block_Support::has_block_support() to ensure it's only added on versions of WordPress which support blocks and to align with other blocks in the plugin.

  • site-kit-wp/includes/Modules/Sign_In_With_Google.php

    Line 309 in 52c7824

    if ( Sign_In_With_Google_Block::can_register() ) {

• Merge Pass full metadata to block registration. #10339

Test Coverage

• Confirm failing E2E tests pass and there are no regressions to other tests.

QA Brief

• The Sign in with Google block should not appear in the block editor when Sign in with Google is not connected.
• The Sign in with Google block should still appear when Sign in with Google is connected.

Changelog entry

• Fix console warning that could appear when Sign in with Google module is not connected.

[tofumatt]

Updating the registerBlockType call to include the block title results in a different error:

["wp.blockEditor.transformStyles Failed to transform CSS. JSHandle@error"],["wp.blockEditor.transformStyles
Failed to transform CSS. JSHandle@error"],["wp.blockEditor.transformStyles Failed to transform CSS.
JSHandle@error"],["wp.blockEditor.transformStyles Failed to transform CSS. JSHandle@error"],
"wp.blockEditor.transformStyles Failed to transform CSS. JSHandle@error"],["wp.blockEditor.transformStyles
Failed to transform CSS. JSHandle@error"],["wp.blockEditor.transformStyles Failed to transform CSS.
JSHandle@error"],["wp.blockEditor.transformStyles Failed to transform CSS. JSHandle@error"]

[tofumatt]

(If this issue's fix is straightforward, including it in main/1.148.0 would be great, but if not that's okay.)

[benbowler]

@tofumatt @aaemnnosttv the registerBlockType function does accept the block name string in it's first param, so there should be no issue with registering the SiwG block as we have been doing.

In newer builds of WP, the processBlockType function is causing the warning we are seeing here. The main difference I can see in this case is that, we only register the block on the PHP side if the module is connected, however we always register the block on the frontend regardless.

Therefore, a solution to this issue is to only enqueue the block script on the frontend if the SiwG module is connected, I have written an IB to this effect.

---

While I was here I noticed that the other block scripts are prevented from being loaded on older WP versions that don't support blocks however I noticed this is not checked for the SiwG block and I've added this to the IB as it's quick to add in here while working on this fix.

[aaemnnosttv]

Thanks @benbowler that makes sense. We should really move to defining the block's scripts and styles in the block.json instead. This way we're only registering it once on the backend. I think maybe we had done both before because we were trying to support versions of WP before block.json support was added, but since we've made that a requirement going forward, that shouldn't be an issue anymore. There have been some changes to the schema over time there of course, but I think this problem goes away if we define the index.js as the editorScript in the block. The index is still needed to register the client-side parts, as can be seen in an example from create-block.

If we do go this route, we should apply it to SiwG too.

[tofumatt]

One of the reasons we register the block on the server is to support a dynamic render function:

site-kit-wp/includes/Modules/Sign_In_With_Google/Sign_In_With_Google_Block.php

Line 68 in c21073c

'render_callback' => array( $this, 'render_callback' ),

Specifying this with block.json would require us being able to use composer autoload for files outside includes/, but I was not clear on how we'd go about this 🤔 But with a PHP file co-located with the block.json we could specify a PHP file that handled block output from block.json, which would be good… though ultimately I'd think we still want/need to specify the render callback for server content rendering anyway 🤔

Ultimately registration is recommended to be done both in PHP and JS, as per: https://developer.wordpress.org/block-editor/reference-guides/block-api/block-registration/

The SiwG block JS should indeed only be registered when the module is active though, that's a good point. 🤦🏻

[benbowler]

Adding "editorScript": "file:./index.js" or "script": "file:./index.js" doesn't resolve this warning. As @tofumatt mentions the docs recommend registering both in PHP and JS and we need the PHP definition as we handle the view rendering in PHP for this block.

I've updated the ticket to include these references in the block.json for future proofing.

[aaemnnosttv]

TL;DR – let's go with the 1 line change for now as in the current PR and address the rest in a follow up (without making changes to the block.json here).

IB ✅ but I'll remove the bit about changing block.json

One of the reasons we register the block on the server is to support a dynamic render function

@tofumatt I'm not suggesting that we don't register it on the server, but that we only register it this way – which is I think the intent we have now, as I said, the client side registerBlockType is still required for providing client-specific elements like React components, we're just enqueuing this as a normal script. What I meant to say is that core should handle the loading/enqueuing of all block assets, we should only need to define them as part of the block.

Adding "editorScript": "file:./index.js" or "script": "file:./index.js" doesn't resolve this warning. As @tofumatt mentions the docs recommend registering both in PHP and JS and we need the PHP definition as we handle the view rendering in PHP for this block.

You may have misunderstood the suggestion. Yes, we still want to register the block on the server side. What I meant was that we shouldn't be enqueuing the block's index.js at all as this will be handled by core if we define it with editorScript. As you've identified, the reason it is warning is because the block is always being registered on the client in the block editor context due to its automatically enqueued JS for the post editor load_contexts, since the server-side registration where the title is provided only happens when the module is connected. The same warning doesn't happen when the module is connected because that server-provided data is passed along to the client. This is what I meant when I say that letting core load block assets should solve this problem.

---

Looking at the IB, the note about changing the condition in setup_assets should prevent the block from being registered before the module is connected, but really I think those assets shouldn't be set up there at all as they should be set up automatically when the block is registered, which already has the proper condition.

Checking my own math, I found that when we remove our registered JS and allow it to be handled by core, it works, however block registration errors because no dependencies are defined and so the call to registerBlockType happens before wp-blocks is loaded (Uncaught TypeError: Cannot read properties of undefined (reading 'registerBlockType')), so there is actually more to do here to address this properly in how we generally approach blocks, but we can handle that in a follow up issue.

[mohitwp]

QA Update ✅

• Tested on dev environment.
• Verified The Sign in with Google block not appear in the block editor when Sign in with Google is not connected.
• Verified The Sign in with Google block appear when Sign in with Google is connected.
• Tested on page and post using full site editing theme.

When SIWG is not connected

Recording.1896.mp4

When SIWG is not connected

Recording.1897.mp4