Block Directory: Use local assets with automatic asset detection

[dd32]

Description

Use a XHR request to the post edit screen to detect what Script/Style assets to insert into the page after Block Installs.

This resolves translations not loading for installed Blocks, and blocks that don't currently load all of the required assets.

See https://core.trac.wordpress.org/ticket/50732#comment:6 for background

How has this been tested?

Manual testing. Installing blocks, making sure things work as expected.

Screenshots

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[ryelle]

This is working well for the blocks I've tried, including some blocks we had other issues with. The only edge case I've found so far is if you install a block, remove it from the page & save (so it uninstalls the block), then try to re-add it, the scripts are technically still on the page, so it's not loaded back in. I think that's sufficiently edge-case, but still wanted to flag it (that whole uninstall-unused-blocks flow is awkward, we'll probably want to revisit that with 5.6 too)

[dd32]

The only edge case I've found so far is if you install a block, remove it from the page & save (so it uninstalls the block), then try to re-add it, the scripts are technically still on the page, so it's not loaded back in.

As long as the scripts are still loaded, skipping inserting them again and proceeding with the block insertion should work, right?

[ryelle]

Well, the block gets unregistered on uninstall, so it would need to be registered again, which is done in the plugin's JS. and if we don't unregister it, it stay in the block library as if it still exists.

[tellthemachines]

Code looks good, just a couple minor comments below.

The e2e tests are failing and I can reproduce the failure locally by running THROTTLE_CPU=4 npm run test-e2e packages/e2e-tests/specs/editor/plugins/block-directory-add.test.js. Perhaps something like await page.waitForSelector here instead of a timeout might fix it?

I tested in Spanish using simple-iframe and verified it loaded the correct version right away, whereas without this change it first loads the English version. Also tried block-a-saurus but it's still loading the English version. embed-block-for-github is also still loading in English and the first time I tried it there was a PHP error on the page (which I stupidly didn't screenshot and haven't been able to reproduce since; hopefully it's nothing).

[dd32]

The e2e tests are failing and I can reproduce the failure locally by running THROTTLE_CPU=4 npm run test-e2e packages/e2e-tests/specs/editor/plugins/block-directory-add.test.js. Perhaps something like await page.waitForSelector here instead of a timeout might fix it?

Unfortunately this wasn't easy to fix, I've worked around it previously in https://github.com/WordPress/block-directory-e2e/blob/master/specs/block-directory.test.js#L104-L108 by adding a networkIdle implementation that waits until all resources are loaded.
I'm not sure if that's the approach that should be taken here too?

I tested in Spanish using simple-iframe and verified it loaded the correct version right away, whereas without this change it first loads the English version.

simple-iframe includes Spanish translations in the plugin, and works well with this change.

Also tried block-a-saurus but it's still loading the English version.
embed-block-for-github is also still loading in English

Unfortunately, WordPress doesn't install translations when Blocks are installed at present. WordPress/wordpress-develop#427 is pending for that.

embed-block-for-github .... and the first time I tried it there was a PHP error on the page (which I stupidly didn't screenshot and haven't been able to reproduce since; hopefully it's nothing).

Any chance that might be in some error logs somewhere? I'm not sure how/what/why that could've been..

[tellthemachines]

Managed to reproduce the error again when I reopened the post I had added the GitHub block to in a new tab:

Notice: Undefined index: github_url in /var/www/html/wp-content/plugins/embed-block-for-github/embed-block-for-github.php on line 112

Warning: Cannot modify header information - headers already sent by (output started at /var/www/html/wp-content/plugins/embed-block-for-github/embed-block-for-github.php:112) in /var/www/html/wp-admin/admin-header.php on line 9

Regarding the e2e test, I reckon it's worth trying the networkIdle approach, otherwise we may never get this to pass on GitHub 😖

[dd32]

Managed to reproduce the error again when I reopened the post I had added the GitHub block to in a new tab:

Awesome, thanks! It doesn't appear to be inserted into the post from this code, but it will appear pre-post-render on development setups, not much we can do about plugin notices. Shouldn't be seen in production.
I had seen something similar from another plugin, but had completely forgotten it.

Regarding the e2e test, I reckon it's worth trying the networkIdle approach, otherwise we may never get this to pass on GitHub 😖

That's what I figured, I was hoping there'd be a simpler approach as I didn't really want to add request-interception for all tests unless it's already in use somewhere (I don't know much about e2e testing, other than having spent far too long debugging this exact problem). @StevenDufresne Do you have any ideas?

[StevenDufresne]

Yep, since we are mocking all the requests, waiting for the install button to be hidden is a good call.

[dd32]

Regarding the e2e test, I reckon it's worth trying the networkIdle approach, otherwise we may never get this to pass on GitHub 😖

That's what I figured, I was hoping there'd be a simpler approach as I didn't really want to add request-interception for all tests unless it's already in use somewhere (I don't know much about e2e testing, other than having spent far too long debugging this exact problem). @StevenDufresne Do you have any ideas?

I misunderstood how these tests were running, and how it mocked requests, I believe 042dfd5 will handle this appropriately. I used .is-root-container to scope it to the editor flow, but I'm not sure that's the right selector to use here, or if it's even needed. Removed that in 25c1e20

[tellthemachines]

Oh wait, I re-ran the tests in interactive mode and the block actually has an error and doesn't make it onto the page at all:

This happens even without CPU throttling.

[ryelle]

Everything's looking good— I tried a bunch of ways I could think of to break things, intentionally triggering fatal errors once the plugin is installed, that sort of thing, and it never broke the editor. We could definitely improve some of the UX in errors, but that's tracked in #23733.

[TimothyBJacobs]

Can we remove assets from the block directory REST API controller now?

[dd32]

Can we remove assets from the block directory REST API controller now?

IMHO Yes, however, it seems likely that it would be re-implemented for WP 5.6 - probably with a different schema though - for making use of block.json provided asset lists. The existing code merged here might remain, or might ultimately be reworked to support that use-case.

[TimothyBJacobs]

Ok, I'm going to drop it then.