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

Do not fatal when trying to enqueue asset from missing manifest location #57

Closed
kadamwhite opened this issue Dec 1, 2023 · 3 comments
Closed
Labels
bug Something isn't working

Comments

@kadamwhite
Copy link
Collaborator

kadamwhite commented Dec 1, 2023

This plugin will currently throw a hard error when we try to load the page if we try to enqueue an asset from a manifest that doesn't exist. We should adjust this behavior so that it logs a warning, but does not actually stop loading the page, because having it completely error prevents quick bisecting between branches that need builds (among other things).

The error we see sometimes:

 Fatal error: Uncaught Error: Asset_Loader\enqueue_asset(): Argument #1 ($manifest_path) must be of type string, null given, called in /wp/wp-content/themes/themename/functions.php on line 139
in /wp/wp-content/plugins/asset-loader/inc/namespace.php on line 170

This is a consequence of themes using this pattern:

/**
 * Check through the available manifests to find the first which includes the
 * target asset. This allows some assets to be loaded from a running DevServer
 * while others load from production files on disk.
 *
 * @param string $target_asset Desired asset within the manifest.
 * @return string|null
 */
function myproject_custom_get_manifest_path( $target_asset ) {
	$manifests = [
		get_template_directory() . '/assets/dist/development-asset-manifest.json',
		get_template_directory() . '/assets/dist/production-asset-manifest.json',
	];
	foreach ( $manifests as $manifest_path ) {
		$asset_uri = \Asset_Loader\Manifest\get_manifest_resource( $manifest_path, $target_asset );
		if ( ! empty( $asset_uri ) ) {
			return $manifest_path;
		}
	}
}


function mytheme_enqueue_scripts() {
	Asset_Loader\enqueue_asset(
		myproject_custom_get_manifest_path( 'my-script-bundle.js' ),
		'my-script-bundle.js',
		[
			'dependencies' => [],
			'handle' => 'my-script-bundle',
		]
	);
}

This code should be improved -- The function which looks up a resource in a series of manifests should not return null if no matching asset is found. But that theme code quality concern still shouldn't cause a hard error because of this plugin.

Proposed change

Within Asset Loader, we should alter the enqueue_asset and register_asset functions to allow them to receive a ?string instead of a string, and add an if ( empty( $manifest_path ) ) { at the start of register_asset so that it will detect missing manifests before we try to register the asset. If the manifest is missing, we should log an informational error to the logs, and return an empty array from register_asset(). (Returning an empty array from that method will cause enqueue_asset to take no further action.)

Steps to reproduce

  • In a vip dev-env environment that is using a theme using this library, (contact me for an example, if HM-internal),
  • ensure the built files are not yet present (e.g. delete build directory or check out non-release branch)
  • try to load a page on the site
  • observe that the error shown above is thrown after no manifest can be found
@kadamwhite kadamwhite added the bug Something isn't working label Dec 1, 2023
kadamwhite added a commit that referenced this issue Dec 8, 2023
#57: Do not fatal if empty/null manifest is passed to registration functions
@tomjn
Copy link
Contributor

tomjn commented Dec 10, 2023

Is his a duplicate of #41?

@kadamwhite
Copy link
Collaborator Author

@tomjn It was!

Fixed in #59

@kadamwhite
Copy link
Collaborator Author

Released as part of 0.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants