feat: enqueued asset changes #3169

kidunot89 · 2024-07-08T21:55:56Z

What does this implement/fix? Explain your changes.

Modifies the enqueuedScripts, and enqueuedStyles field on the ContentNode to better simulate the rendering of a content node a WordPress theme, as well as sorting the results in the correct order for rendering.
The primary purpose of this change is to enable and simplify the process of render an editor page in a headless application on a 1:1 scale with the WordPress theme. CSS/JS scripts and all.
ScriptLoadingGroupEnum enum type implemented and assigned as the return type for the EnqueuedScript type's new location field.
Adds the group field to the EnqueuedAsset type.
Adds the location field to the EnqueuedScript type.
All self::IS_NULL, self::IS_FALSY, self::NOT_NULL, and self::NOT_FALSY calls in tests replaced with static::IS_NULL, static::IS_FALSY, static::NOT_NULL and static::NOT_FALSY to ensure the youngest definition is always returned. Moved to chore: WPGraphQLTestCase constant usage updated for extendability #3191

Does this close any currently open issues?

Any relevant logs, error output, GraphiQL screenshots, etc?

Any other comments?

…

Where has this been tested?

Operating System: …

WordPress Version: …

qlty-cloud-legacy · 2024-07-08T21:56:32Z

src/Type/ObjectType/EnqueuedScript.php

+						'description' => __( 'The loading strategy to use on the script tag', 'wp-graphql' ),
+						'resolve'     => static function ( \_WP_Dependency $script ) {
+							if ( ! isset( $script->extra['group'] ) ) {
+								return 0;


Avoid too many return statements within this method.

Should this be on the interface?

Also, defaulting to HEADER only seems true for scripts, but per the above link, a false value is NO_GROUP. Not sure if that's a helpful demarcation for non-script assets, but it does respect WP as source-of-truth

Gotcha, I've copied the group field to the EnqueuedAsset type with a return type to Integer. Then renamed the group field to location in the EnqueuedScript file.

Not sure I follow the change. What is the difference between group and location?

group is the group level that can be applied to all _WP_Dependencies, and location is the enum representation of this value exclusive to scripts being either HEADER for 0 and FOOTER for 1.

src/Type/ObjectType/EnqueuedScript.php

qlty-cloud-legacy · 2024-07-08T21:56:32Z

src/Type/InterfaceType/ContentNode.php

+	 *
+	 * @return array
+	 */
+	public static function get_enqueued_scripts_handles( $queue ) {


Similar blocks of code found in 2 locations. Consider refactoring.

qlty-cloud-legacy · 2024-07-08T21:56:32Z

src/Type/InterfaceType/ContentNode.php

+		return array_values( array_unique( $handles ) );
+	}
+
+	public static function get_enqueued_styles_handles( $queue ) {


Similar blocks of code found in 2 locations. Consider refactoring.

qlty-cloud-legacy · 2024-07-08T21:56:32Z

src/Type/InterfaceType/ContentNode.php

+	 *
+	 * @return array
+	 */
+	public static function get_enqueued_scripts_handles( $queue ) {


Function get_enqueued_scripts_handles has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.

justlevine · 2024-07-11T18:33:52Z

src/Type/InterfaceType/ContentNode.php

 						'resolve' => static function ( $source, $args, $context, $info ) {
+							// Simulate WP template rendering
+							ob_start();
+							wp_head();


This will be incompatible with eventual FSE support, and break several existing WPGraphQL extensions (e.g. seo). I haven't reviewed the PR for real, so without knowing the full goal I gotta hope there's a different way to accomplish this.

We'll maybe not the FSE but plain-old Gutenburg and editor pages, yes.

Also it runs on the resolvers for both enqueuedStyles and enqueuedScripts, which means these do_action() and apply_filters() are getting triggered multiple times too... What's the performance hit with this change?

Nothing that I've noticed, but I'm just using WC and some WC premium plugins, and while WC emits a lot of localize scripts for their pages I've haven't seen any slow downs in the query speed due to this change.

src/Type/InterfaceType/EnqueuedAsset.php

qlty-cloud-legacy · 2024-07-11T21:22:54Z

src/Type/InterfaceType/EnqueuedAsset.php

+						'description' => __( 'The loading group to which this asset belongs.', 'wp-graphql' ),
+						'resolve'     => static function ( $asset ) {
+							if ( ! isset( $asset->extra['group'] ) ) {
+								return 0;


Avoid too many return statements within this method.

qlty-cloud-legacy · 2024-07-11T21:22:55Z

src/Type/InterfaceType/EnqueuedAsset.php

+								return 0;
+							}
+
+							return absint( $asset->extra['group'] );


Avoid too many return statements within this method.

qlty-cloud-legacy · 2024-07-11T21:22:55Z

src/Type/InterfaceType/ContentNode.php

+	 *
+	 * @return string[]
+	 */
+	public static function get_enqueued_asset_handles( array $queue, $wp_assets ) {


Function get_enqueued_asset_handles has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.

coveralls · 2024-07-11T21:36:18Z

coverage: 84.205% (+0.03%) from 84.172%
when pulling f6adc45 on kidunot89:feat/enqueued-asset-changes
into 66aaf9a on wp-graphql:develop.

qlty-cloud-legacy · 2024-07-25T23:12:24Z

src/Type/InterfaceType/ContentNode.php

+	 *
+	 * @return string[]
+	 */
+	public static function get_enqueued_asset_handles( array $queue, $wp_assets ) {


Function get_enqueued_asset_handles has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.

qlty-cloud-legacy · 2024-08-07T22:11:06Z

src/Utils/Utils.php

+	 *
+	 * @return string[]
+	 */
+	public static function flatten_enqueued_assets_list( array $queue, $wp_assets ) {


Function flatten_enqueued_assets_list has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.

…ution implemented

src/Type/ObjectType/EnqueuedScript.php

qlty-cloud-legacy · 2024-08-19T14:42:35Z

Code Climate has analyzed commit f6adc45 and detected 8 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	8

View more on Code Climate.

Cherrypicked from wp-graphql#3169 Co-authored-by: Geoff Taylor <[email protected]>

kidunot89 requested review from jasonbahl and justlevine July 8, 2024 21:55

qlty-cloud-legacy bot reviewed Jul 8, 2024

View reviewed changes

justlevine reviewed Jul 11, 2024

View reviewed changes

src/Type/InterfaceType/EnqueuedAsset.php Outdated Show resolved Hide resolved

justlevine added the compat: breaking change This is a breaking change to existing functionality label Jul 11, 2024

qlty-cloud-legacy bot reviewed Jul 11, 2024

View reviewed changes

kidunot89 force-pushed the feat/enqueued-asset-changes branch from 1f70926 to af5bbc5 Compare July 25, 2024 23:11

qlty-cloud-legacy bot reviewed Jul 25, 2024

View reviewed changes

kidunot89 force-pushed the feat/enqueued-asset-changes branch from af5bbc5 to 6188d78 Compare August 7, 2024 18:30

qlty-cloud-legacy bot reviewed Aug 7, 2024

View reviewed changes

kidunot89 added 13 commits August 8, 2024 14:46

fix: enqueued asset resolution fix

4586c3d

fix: Page rendering simulation improved

6b31091

fix: Unnecessary changes restored, some test cleanup

a6276a8

devops: final self keywords replaced

97867ab

fix: unnecessary conditional removed

7cc04ba

fix: EnqueuedScript and EnqueuedStylesheet dependencies field resol…

337ad78

…ution implemented

chore: Linter and PHPStan compliance met

f7738ad

fix: General improvements and bugfixes

6c0aefe

chore: removed unnecessary changes

7e04be2

fix: unneeded import removed

ff0d842

fix: import statement restored with conditional

2627ff5

devops: EnqueuedScriptsTest fixed

a7d0ae9

chore: Linter compliance met

d7b71a5

kidunot89 force-pushed the feat/enqueued-asset-changes branch from 3872ec5 to d7b71a5 Compare August 8, 2024 18:47

fix: location better resolved

e3f4d27

qlty-cloud-legacy bot reviewed Aug 9, 2024

View reviewed changes

src/Type/ObjectType/EnqueuedScript.php Outdated Show resolved Hide resolved

src/Type/ObjectType/EnqueuedScript.php Outdated Show resolved Hide resolved

src/Type/ObjectType/EnqueuedScript.php Outdated Show resolved Hide resolved

kidunot89 requested a review from justlevine August 9, 2024 21:35

fix: unnecessary code change removed

9a5efb9

kidunot89 added 2 commits August 16, 2024 17:02

chore: Linter compliance met

c11f07d

devops: Some changes to test reverted

f6adc45

justlevine added a commit to justlevine/wp-graphql that referenced this pull request Aug 24, 2024

feat: expose *Asset group and location fields

5eda24a

Cherrypicked from wp-graphql#3169 Co-authored-by: Geoff Taylor <[email protected]>

justlevine mentioned this pull request Aug 24, 2024

feat: expose EnqueuedAsset.group and EnqueuedScript.location to schema #3196

Merged

feat: enqueued asset changes #3169

Are you sure you want to change the base?

feat: enqueued asset changes #3169

Uh oh!

Conversation

kidunot89 commented Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any relevant logs, error output, GraphiQL screenshots, etc?

Any other comments?

Where has this been tested?

Uh oh!

qlty-cloud-legacy bot Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qlty-cloud-legacy bot Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidunot89 Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qlty-cloud-legacy bot Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qlty-cloud-legacy bot Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qlty-cloud-legacy bot commented Aug 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kidunot89 commented Jul 8, 2024 •

edited

Loading

kidunot89 Jul 11, 2024 •

edited

Loading

coveralls commented Jul 11, 2024 •

edited

Loading