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

Enqueued scripts should use plugins_url() instead of plugin_dir_url() #1647

Closed
westonruter opened this issue Nov 11, 2024 · 7 comments · Fixed by #1761
Closed

Enqueued scripts should use plugins_url() instead of plugin_dir_url() #1647

westonruter opened this issue Nov 11, 2024 · 7 comments · Fixed by #1761
Assignees
Labels
Good First Issue Issue particularly suitable to be worked on by new contributors [Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Plugin] Web Worker Offloading Issues for the Web Worker Offloading plugin. [Type] Enhancement A suggestion for improvement of an existing feature

Comments

@westonruter
Copy link
Member

Feature Description

When the URLs for JS files are generated in the plugins, the plugin_dir_url() function is used. Its output is concatenated with the relative path:

wp_json_encode( add_query_arg( 'ver', OPTIMIZATION_DETECTIVE_VERSION, plugin_dir_url( __FILE__ ) . 'detect.js' ) ),

However, this is not ideal because the resulting URL is not directly filterable. It would be better to use plugins_url():

- add_query_arg( 'ver', OPTIMIZATION_DETECTIVE_VERSION, plugin_dir_url( __FILE__ ) . 'detect.js'
+ plugins_url( add_query_arg( 'ver', OPTIMIZATION_DETECTIVE_VERSION, 'detect.js' ), __FILE__ )

In this way, the plugins_url filter can be used to potentially rewrite the URL to point to a CDN, for example.

See instances: https://github.com/search?q=repo%3AWordPress%2Fperformance%20%2Fplugin_dir_url%5C(%2F&type=code

@westonruter westonruter added [Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Plugin] Web Worker Offloading Issues for the Web Worker Offloading plugin. [Type] Enhancement A suggestion for improvement of an existing feature Good First Issue Issue particularly suitable to be worked on by new contributors labels Nov 11, 2024
@github-project-automation github-project-automation bot moved this to Not Started/Backlog 📆 in WP Performance 2024 Nov 11, 2024
@westonruter westonruter moved this from Not Started/Backlog 📆 to To Do 🔧 in WP Performance 2024 Nov 12, 2024
@felixarntz
Copy link
Member

@westonruter I'm not sure I agree with your assessment:

  • plugin_dir_url() wraps plugins_url(), so the URL up to the plugin's own directory is still filterable.
  • This means e.g. changing to point to a CDN works just as well with plugin_dir_url().
  • I don't see any benefit of being able to filter the remainder of the URL (the path relative to the plugin's directory). You can't (or at the very least shouldn't) change the contents of a plugin from the outside, so being able to filter that part makes no sense to me. It's safer not to allow filtering it.

@westonruter
Copy link
Member Author

A couple additional use cases for being able to filter the entire URL:

  • Replace a minified JS or CSS file with a non-minified one, or vice-versa. This requires adding/removing .min from the URL path. (Example filter from Jetpack.)
  • Add a cache busting query parameter.

Jetpack favors plugins_url() over plugin_dir_url() with 62 files vs 8 files, respectively.

Maybe more importantly, Gutenberg also favors plugins_url() over plugin_dir_url(), with 32 files vs 5 files, respectively.

@SohamPatel46
Copy link
Contributor

Hello 👋
I would like to work on this issue and update the url function, if its still relevant here.
Thanks.

@felixarntz
Copy link
Member

Thanks @SohamPatel46 for the offer! I think for now we're still discussing whether or not we should make this change. Curious if you have any thoughts on it.

One additional benefit I see with plugin_dir_url() over plugins_url() is that it's better aligned with plugin_dir_path(), which is also commonly needed in plugins. There is no plugins_path() function, so plugins_url() wouldn't be the same. Regarding the ability to filter the entire URL that you mention @westonruter, I guess this means it wouldn't be possible for the CSS/JS file path. Granted, this is probably less important than filtering the URL, but still a potential drawback and at the very least an inconsistency.

From a consistency and ergonomics perspective I'm in favor of keeping plugin_dir_url(), as it aligns with plugin_dir_path(). Additionally, plugins_url() is a lower-level function (as indicated by that it's being used within plugin_dir_url()), and its name is less intuitive to the purpose we'd be using it for, which is to get a specific plugin's asset URL, whereas "plugins URL" sounds more about a URL within the overall wp-content/plugins directory - which still works of course, but it's less intuitive.

@SohamPatel46
Copy link
Contributor

Hello @felixarntz,

As mentioned by @westonruter Jetpack and Gutenberg mostly uses plugins_url() whenever we want a path appended at the end of the URL. So, I think that should be a good practice to use plugins_url() whenever we want to append a path to the URL, rather then manual concatenation to plugin_dir_url()'s output.

Also I think plugin_dir_url() internally uses plugins_url() with keeping an empty path ( ' ' ) as first argument. Ref. So, possibly I dont see any issue upgrading to plugins_url() function.

@felixarntz
Copy link
Member

Makes sense, the transition is certainly possible. Though what do you think about the point I mention on ergonomics?

In any case, it looks like both of you think plugins_url() works better for this, so I consider myself outvoted. :)

@westonruter westonruter changed the title Enqueued scripts should use plugins_url() instead of plugin_dir_url() Enqueued scripts should use plugins_url() instead of plugin_dir_url() Dec 18, 2024
@SohamPatel46
Copy link
Contributor

@felixarntz On ergonomics point, Though I might not have a suitable verdict on it, but I believe the purpose of the function should matter more than its name. In our case, since plugins_url fits for the purpose more.

Heads up - I am starting work on this, since I have a thumbs up. Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Issue particularly suitable to be worked on by new contributors [Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Plugin] Web Worker Offloading Issues for the Web Worker Offloading plugin. [Type] Enhancement A suggestion for improvement of an existing feature
Projects
Status: Done 😃
Development

Successfully merging a pull request may close this issue.

3 participants