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

Woo Analytics: use core WP function to enqueue script #13173

Merged
merged 2 commits into from Aug 15, 2019

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Aug 2, 2019

Related: #13039

Changes proposed in this Pull Request:

  • For third-parties to be able to interact with our tracking script, it is easier if it is registered using core WP functions instead of just added to wp_head.

Using wp_enqueue_script makes it easier to dequeue the file if needed, or modify its output with the script_loader_tag filter (one could add an async parameter for example, or add extra data attributes like the Cookiebot plugin.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • @psealock I'd appreciate your review on this, to be sure I am not missing anything obvious with this enqueue. Thank you!

Testing instructions:

  • Install and activate the WooCommerce plugin on your site.
  • Connect Jetpack to WordPress.com
  • With or without this patch, you should see the same file being enqueued in the head: <script type='text/javascript' src='https://stats.wp.com/s-201931.js'></script> (note that the exact filename changes week after week).

Proposed changelog entry for your changes:

  • WooCommerce Analytics: use core WP function to enqueue script

Related: #13039

For third-parties to be able to interact with our tracking script, it is easier if it is registered using core WP functions instead of just added to wp_head.

Using wp_enqueue_script makes it easier to dequeue the file if needed, or modify its output with the script_loader_tag filter (one could add an async parameter for example, or add extra data attributes like the Cookiebot plugin.
@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Pri] Normal [Feature] WooCommerce Analytics labels Aug 2, 2019
@jeherve jeherve added this to the 7.7 milestone Aug 2, 2019
@jeherve jeherve requested a review from psealock August 2, 2019 16:22
@jeherve jeherve requested a review from a team as a code owner August 2, 2019 16:22
@jeherve jeherve self-assigned this Aug 2, 2019
@jetpackbot
Copy link

jetpackbot commented Aug 2, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: September 3, 2019.
Scheduled code freeze: August 27, 2019

Generated by 🚫 dangerJS against d184acf

@psealock
Copy link
Contributor

psealock commented Aug 5, 2019

Hi @jeherve, thanks for the ping. I'm trying to test this locally but am having trouble turning off development mode so I can connect to wp.com.

I tried these with no luck:

define( 'JETPACK_DEV_DEBUG', false );
add_filter( 'jetpack_development_mode', '__return_false' );

My url is http://tangaroa.ngrok.io. Do you have any suggestions?

@jeherve
Copy link
Member Author

jeherve commented Aug 5, 2019

@psealock What happens if you just comment out all those filters / constants? By default, if your site is publicly accessible, you should be offered the option to connect to WordPress.com.

psealock
psealock previously approved these changes Aug 6, 2019
Copy link
Contributor

@psealock psealock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jeherve, this a more legitimate way to load the script. I tested locally and the script is loading correctly. I'm not forseeing any issues around dependencies or load order. Requests for t.gif are firing as they should.

@psealock
Copy link
Contributor

psealock commented Aug 6, 2019

By default, if your site is publicly accessible, you should be offered the option to connect to WordPress.com.

Hmm. Something, which I can't find, is applying the jetpack_development_mode filter. I ended up hardcoding is_development_mode to false.

@kbrown9
Copy link
Member

kbrown9 commented Aug 8, 2019

I tested this change using the provided testing instructions, and the file was enqueued as expected.

Do you think there's any value in adding the async attribute using the script_loader_tag filter in this PR? Or should that be a separate PR? Google's Page Speed Insights tool flags the https://stats.wp.com/s-2019xx.js file as a render-blocking resource now that the async attribute has been removed.

@psealock
Copy link
Contributor

psealock commented Aug 8, 2019

Do you think there's any value in adding the async attribute

Good catch @kbrown9! Yes, definitely.

@jeherve
Copy link
Member Author

jeherve commented Aug 8, 2019

@kbrown9 That's a good point, you're right! I've just pushed d184acf to add a utility we can reuse in the future to make more scripts async if needed.

@kbrown9
Copy link
Member

kbrown9 commented Aug 8, 2019

I tested this PR with commit d184acf using the provided testing instructions. The file was enqueued in the head, and the file's script tag included the async attribute, as expected!

@kbrown9 kbrown9 added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Aug 8, 2019
@dereksmart dereksmart merged commit d7ddbf3 into master Aug 15, 2019
@dereksmart dereksmart deleted the update/woo-analytics-enqueue branch August 15, 2019 15:57
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 15, 2019
jeherve added a commit that referenced this pull request Aug 23, 2019
jeherve added a commit that referenced this pull request Aug 27, 2019
* 7.7 changelog: initial set of changes

* Changelog: add #13154

* Changelog: add #13134

* Changelog: add #12699 and many others

* Changelog: add #13127

* Changelog: add #13167

* Changelog: add #13225

* Changelog: add #13179

* Changelog: add #13173

* Changelog: add #13232

* Changelog: add #13137

* Changelog: add #13172

* Changelog: add #13182

* Changelog: add #13200

* Changelog: add #13244

* Changelog: add #13267

* Changelog: add #13204

* changelog: add #13205

* Changelog: add #13183

* Changelog: add #13278

* Changelog: add #13162

* Changelog: add #13268

* Changelog: add #13286

* Changelog: add #13273

* Changelog: add #12474

* Changelog: add #13085

* Changelog: add #13266

* Changelog: add #13306

* Changelog: add #13311

* Changelog: add #13302

* Changelog: add #13196

* Changelog: add #12733

* Changelog: add #13261

* Changelog: add #13322

* Changelog: add #13333

* Changelog: add #13335
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] WooCommerce Analytics [Pri] Normal [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants