FSE: require asset files (not _once)

[sirreal]

Changes proposed in this Pull Request

require should be used for asset files not require_once. See WordPress/gutenberg#18599

Illustration of the issue, note the second require_once returns true.

php > var_export( require_once( __DIR__ . '/global-styles.asset.php' ) );
array (
  'dependencies' =>
  array (
    0 => 'lodash',
    1 => 'wp-api-fetch',
    2 => 'wp-components',
    3 => 'wp-compose',
    4 => 'wp-data',
    5 => 'wp-dom-ready',
    6 => 'wp-edit-post',
    7 => 'wp-element',
    8 => 'wp-i18n',
    9 => 'wp-keycodes',
    10 => 'wp-plugins',
    11 => 'wp-polyfill',
  ),
  'version' => 'e53af81a8e55ea0235ea0da817d6aad4',
)
php > var_export( require_once( __DIR__ . '/global-styles.asset.php' ) );
true

Compare to require:

php > var_export( require( __DIR__ . '/global-styles.asset.php' ) );
array (
  'dependencies' =>
  array (
    0 => 'lodash',
    1 => 'wp-api-fetch',
    2 => 'wp-components',
    3 => 'wp-compose',
    4 => 'wp-data',
    5 => 'wp-dom-ready',
    6 => 'wp-edit-post',
    7 => 'wp-element',
    8 => 'wp-i18n',
    9 => 'wp-keycodes',
    10 => 'wp-plugins',
    11 => 'wp-polyfill',
  ),
  'version' => 'e53af81a8e55ea0235ea0da817d6aad4',
)
php > var_export( require( __DIR__ . '/global-styles.asset.php' ) );
array (
  'dependencies' =>
  array (
    0 => 'lodash',
    1 => 'wp-api-fetch',
    2 => 'wp-components',
    3 => 'wp-compose',
    4 => 'wp-data',
    5 => 'wp-dom-ready',
    6 => 'wp-edit-post',
    7 => 'wp-element',
    8 => 'wp-i18n',
    9 => 'wp-keycodes',
    10 => 'wp-plugins',
    11 => 'wp-polyfill',
  ),
  'version' => 'e53af81a8e55ea0235ea0da817d6aad4',
)

Testing instructions

• Ensure the modified FSE plugin parts continue to work as expected.

[matticbot]

Test live: https://calypso.live/?branch=update/fse-require-assets

[matticbot]

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

[noahtallen]

I feel like there are more asset includes than the ones in the PR so far. I don't really understand why it needs to change, but I trust you and everyone else on that issue about it :)

[sirreal]

I feel like there are more asset includes than the ones in the PR so far.

It's the require_once and include_once specifically for the asset files that are problematic. It may be the case that, more generally, *_once may not be what we want with the assignment form:

$LHS = require_once 'RHS.php';

I don't really understand why it needs to change, but I trust you and everyone else on that issue about it :)

It's problematic with asset files becuase find these on the right hand side of assignments:

$assets = require_once 'my-asset-file.php';

Becuase it's an assignment and not simply bringing something into scope (the typical way we want to use require_once), we should use require in order to get consistent results. With require_once, $assets will be the array we expect the first time, and true after that. true isn't the value you're looking for 🧙

[noahtallen]

Makes a lot more sense now, thanks! It would be really nice (in the future) if we could abstract the asset stuff out of each of the sub-modules. The code is so similar across all of them, and yet each tend to do slightly different things which makes it hard to know exactly what to add when you are creating a new sub-module.

[matticbot]

Caution: This PR affects files in the FSE Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D44003-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2

[sirreal]

I've rebased and the automated FSE builds are working again. This needs testing and review if anyone can spare a moment 🙇‍♂️

[ockham]

Looking good to me, and the WP.com diff seems to test fine.

Let's wait for the mobile e2e tests to re-run.