Remove support for delayed inline scripts#81
Conversation
|
I think I've got most of this updated, but am still seeing unit test failures with the following test: The nested aliases are being printed with a Also, while reviewing the logic for this, I noticed that we're duplicating a lot of the same checks in |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@joemcgill This looks solid so far, though I think two things are missing (see below).
…ripts associated with handle or its dependents.
This adds a new private method, `WP_Scripts::filter_eligible_strategies` that is responsible for recursively eliminating possible loading strategies. This removes duplication of logic that previously was in place for checking the eligible strategy of a handle with checking for whether dependents could be deferred. This also removes `WP_Scripts::has_only_deferrable_dependents`, which is no longer needed. Unit tests for `has_only_deferrable_dependents` have been updated to test `filter_eligible_strategies` instead.
|
While reviewing the changes we were making to remove support for delayed inline scripts, I noticed that we were often duplicating the same logic for checking a specific handle that we were using to check whether dependents could be deferred. I've refactored the logic that determines the eligible loading strategy in a4eeb98. This simplifies things quite a bit by recursively using a process of elimination to filter out eligible strategies for a handle and consolidates all the logic to a new private method, |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@joemcgill I only have one small nit-pick regarding the has_inline_script() method, but regarding your additional changes, I have a few more concerns. I am unsure this is a good time to implement a notable refactoring like this, since the logic is quite complex and as far as I can tell you didn't just refactor, but also changed the underlying logic in a few places. I left a few more specific comments below.
Unless there's a good reason for the logic changes, it would be safer to focus only one purely the refactoring, since we're very close to an intended commit. Maybe I'm misunderstanding some bits, but it overall strikes me as a risky change to make this late.
| foreach ( $dependents as $dependent ) { | ||
| // Bail early once we know the eligible strategy is blocking. | ||
| if ( empty( $eligible ) ) { | ||
| return array(); | ||
| } | ||
|
|
||
| $eligible = $this->filter_eligible_strategies( $dependent, $eligible, $checked ); | ||
| } |
There was a problem hiding this comment.
I'm not completely sure this logic is correct. The way it is written it always returns the eligible strategies for the last dependent, unless any of the dependents returned an empty array.
This may not be a common problem in practice, but I think it is possible for one dependent to e.g. return array( 'defer' ) and another dependent later to return array( 'async' ) or array( 'async', 'defer' ). Shouldn't the return value then be the stricter one, i.e. array( 'defer' )?
There was a problem hiding this comment.
We’re passing the array of eligible strategies to this function, which then gets filtered by the characteristics of the dependent, so the filtering is cumulative.
There was a problem hiding this comment.
Thanks for clarifying @joemcgill, that makes sense. Just a recommendation then from my end would be to make $eligible the first parameter of that method, as that makes it more intuitive that that value is what's being filtered in the method.
There was a problem hiding this comment.
The only required param in this method is the first one, $handle, so I think it's best to leave that in the first position.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@joemcgill After your clarifications, this LGTM!
Please review my outstanding feedback in #81 (review) for consideration, but those are more nit-picks than blockers.
joemcgill
merged commit b7d362d
into
feature/enhance-wp-scripts-api-with-a-loading-strategy
3 participants
This removes support for delayed inline scripts when used in conjunction with an
asyncordeferloading strategy. Unit tests are updated to reflect this change in behavior.Fixes #69.