-
Notifications
You must be signed in to change notification settings - Fork 1
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
Enable multiple registrations #29
base: main
Are you sure you want to change the base?
Conversation
src/Assets/Asset.php
Outdated
* | ||
* @return static | ||
*/ | ||
public function __call( $method, $args ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is smart and saves some lines of code, but I do not think makes for readable code.
I would unroll the __call
method and explicitly create each method that is now part of the @method
annotation.
This should likely not be a gateway to all wp_
functions, but to a limited, controlled and tested set of them.
What would this do if I call $asset->footer
?
If you end up having to maintain a safe-list of functions that can be called, you're better served by an explicit API made of concrete methods.
This looks pretty great (once Luca's comment is addressed) 🕺 |
755bb39
to
23a3b6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize that I did not think/catch this sooner, but I think this needs a bit more discussion/thought. I'm happy to jump on a call next week if it'd be helpful to talk it out.
Right now, as I re-review it a second time, my spidey senses are tingling - probably based on the unfurling of that __call()
method and it trigging the appropriate level of attention that I was missing before.
Question A: Should an asset know how to output itself, dequeue itself, etc?
Probably. I don't dislike the movement of some of that awareness and execution into the Asset
class itself. However, it does feel a bit off to have the bulk of the orchestration remain in the Assets
library while just pieces going to the Asset
library.
Question B: Is there a deeper reason for the redundant methods being created that I'm not understanding?
The simple answer to this is probably yes, but as of right now I'm not following. I think seeing register()
, do_register()
, and register_asset()
is an incredibly confusing thing to someone without them digging into the code to understand why there's a distinction there. I, personally, don't know why we've added the two extra methods.
If we do want to have the asset know how to register itself in WP; know how to enqueue itself in WP; etc, those should be done in protected
methods so the API methods that users interact with are restricted to enqueue()
, dequeue()
, and register()
.
Question C: Can you share the thing this is trying to solve in a video walkthrough or something or in a call with me?
I believe I am misunderstanding the purpose. Is the goal to be able to allow the following?
Asset::add( 'thing', /* whatever */ )
->register();
// Do some stuff.
Assets::get( 'thing' )
->dequeue()
->add_dependency( 'kdfdfdfdf' );
->do_enqueue();
That's the only thing I can think of. If that is the case (which I'm guessing it is not), I'm not a huge fan of that and I think there's other ways to accomplish that sort of thing.
TL;DR:
I believe I'm misunderstanding the goal that this changeset aims to accomplish now that I've reviewed it a second time. I'd love to chat!
public function print_asset() { | ||
$method = 'wp_print_' . $this->get_script_or_style() . 's'; | ||
$method( $this->get_slug() ); | ||
return $this; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing to have along with do_print()
. Is there a reason to keep this separated from do_print()
? Should we change the behavior of do_print()
to call wp_print_*
directly so that we don't have the extra method that might get confused with print()
?
public function enqueue_asset() { | ||
$method = 'wp_enqueue_' . $this->get_script_or_style(); | ||
$method( $this->get_slug() ); | ||
return $this; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be confused with enqueue()
, which should be doing the enqueueing anyway. I'm not sure what the value from this is? The Asset::enqueue()
method should be doing this already.
The question I have is this: What is broken about enqueue()
(ultimately Assets::enqueue()
and Assets::do_enqueue()
) that is causing the creation of this method? As of right now, it feels redundant and confusing.
I totally admit that I might be missing something and there is a very very valid reason. I'm just unsure I see it without some tips/hints on what to look at.
The concept behind the Assets library
The goal is to tell WordPress about an asset so that it can be registered and enqueued at the right moment so the dev doesn't need to go to the precise location and call register and enqueue functions directly. If the dev wants to do that, then the Asset library isn't really adding much benefit.
The Asset
is meant to hold information about the asset itself and Assets
was for orchestrating the output. That was the original idea, at least.
public function register_asset( string $url = '', array $dependencies = [], string $version = null, $in_footer_or_media = 'all' ) { | ||
$url = $url ? $url : $this->get_url(); | ||
$dependencies = $dependencies ? $dependencies : $this->get_dependencies(); | ||
$version = $version ?? $this->get_version(); | ||
$in_footer_or_media = $in_footer_or_media ? $in_footer_or_media : ( $this->is_js() ? $this->is_in_footer() : $this->get_media() ); | ||
|
||
$method = 'wp_register_' . $this->get_script_or_style(); | ||
$method( $this->get_slug(), $url, $dependencies, $version, $in_footer_or_media ); | ||
return $this; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels redundant with register()
. If we really need this logic in here, is it best to replace the functionality of register()
? To skip around that method sorta breaks the library's purpose.
The primary funcitonality that is being introduced is allowing you to make multiple registrations just by using the API exposed.
Previously, because internally there were calls to wp_script_is or wp_style_is even if you removed the asset altogether you could not register more than once.
Registering more than once can have some advantages - listing a couple that pop on the top of my head.
Now you can modify your original's script dependencies to include your new script. Making managing when your script should be included easy. Wherever my original script is included, also please include my extension.
Also this PR introduces a magic method which enables us to call script or style methods from WP on the asset instance itself. You only need to call the method without the
wp_
prefix and replacing withasset
wherever thescript
orstyle
would be.So now you can do:
See an example of a snippet we had to provide support to achieve the functionality being added here:
https://lw.slack.com/archives/C01SBD5T03V/p1732799298015619?thread_ts=1732270965.841129&cid=C01SBD5T03V