google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.21k stars 278 forks source link

Event provider JS doesn't load on production build #8955

Open aaemnnosttv opened 1 week ago

aaemnnosttv commented 1 week ago

Bug Description

The event provider JS loaded on the front end for its respective supported plugin integration for enhanced conversion tracking does not load when using a production build of Site Kit. It does work on development builds, but not intentionally.

Steps to reproduce

  1. Use a production build of SK
  2. Setup GA and enable enhanced conversion tracking
  3. Enable a supported plugin
  4. View frontend
  5. See 404 for event provider JS

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

benbowler commented 1 week ago

@10upsimon I actually had a similar issue on my Consent Mode refactor issue #8384 today, this is how I fixed it.

tofumatt commented 1 week ago

IB ✅

aaemnnosttv commented 1 week ago

@tofumatt @10upsimon @benbowler

  • change the logic within the register_script() method to instead use the Manifest::get() method, passing self::CONVERSION_EVENT_PROVIDER_SLUG as the parameter, to obtain the filename

This would work but isn't necessary as Script already handles the application of the file in the Manifest automatically. Ultimately, this is the root cause for the issue as you'll notice no assets touch the Manifest directly yet they all load the right URLs for the build. We simply need to update the event provider scripts to follow the same approach as every other asset rather than treating these as a special case unnecessarily.

10upsimon commented 1 week ago

Thanks @aaemnnosttv I've simplified this and handed back to you.

aaemnnosttv commented 1 week ago
  • For each of the event providers within the includes/Core/Conversion_Tracking/Conversion_Event_Providers folder, change the logic within the register_script() method to instead pass just the self::CONVERSION_EVENT_PROVIDER_SLUG constso as to handle correct Manifest behavior when surfacing the asset URL
  • Remove the prepended 'gsk-cep-' string from the handle passed to Script(), this will ensure correct handling of the Manifest entry in the Script class.

@10upsimon while this will have the intended outcome as far as loading the script is concerned, it's changing the handle to be generic. E.g. gsk-cep-mailchimp will become mailchimp, which as a (global) WP script handle, is not something SK should be using but this might not be obvious due to the abstraction of our Script; it does not add any kind of namespacing, etc to the given handle. With that said, we should update both the handle and the entry name (key) to reference the same value (according to the AC) since the entry name is what the resulting manifest is keyed by.

Since the entry name also defines the default resulting filename, there's no need to add the prefix to the output if it's included in the entry name.

aaemnnosttv commented 1 week ago

IB ✅

mohitwp commented 6 days ago

QA Update ✅

JS files

![image](https://github.com/google/site-kit-wp/assets/94359491/205a03b5-950f-4e8f-97f0-843866a13829) ![image](https://github.com/google/site-kit-wp/assets/94359491/4cf674ad-e5b4-419b-b19e-b8334c1f1bb0) ![image](https://github.com/google/site-kit-wp/assets/94359491/a3d24af3-d90e-4cfd-ba70-03abcc17af14)

WPFORMS

When only Analytics is connected ![image](https://github.com/google/site-kit-wp/assets/94359491/444c97aa-e02a-4e39-97e4-dc0a35aab5a4) When both analytics and Ads module connected ![image](https://github.com/google/site-kit-wp/assets/94359491/ebce2433-27f3-4e3d-9817-8ce9fe56f429) ![image](https://github.com/google/site-kit-wp/assets/94359491/8d9869cc-c141-432b-a651-6b557d4cc399) https://github.com/google/site-kit-wp/assets/94359491/75588f69-512a-420f-aee3-da18d52d2048

Woocommerce

![image](https://github.com/google/site-kit-wp/assets/94359491/0704950c-e335-431f-bd15-ff2f8d216f3e) https://github.com/google/site-kit-wp/assets/94359491/4f016f17-81bb-4a9a-9ed7-34f7ea44b6e3 `event : add to cart`
When only analytics is connected ![image](https://github.com/google/site-kit-wp/assets/94359491/7c47a6e0-ae1e-42ed-bd48-a18c92742618) Both analytics and Ads module connected ![image](https://github.com/google/site-kit-wp/assets/94359491/f4a303da-bb0f-4dab-b74b-fc4fbcdf7314) ![image](https://github.com/google/site-kit-wp/assets/94359491/8f235024-5c1d-4f20-a204-bbc7e23da50e)
**event : Purchase**
![image](https://github.com/google/site-kit-wp/assets/94359491/5a8eb061-6366-4b44-996e-b4cfd265755e) When both analytics and Ads module connected ![image](https://github.com/google/site-kit-wp/assets/94359491/f127fee7-96cb-426a-b8a3-c8e6984a9935) ![image](https://github.com/google/site-kit-wp/assets/94359491/f0a6da32-ccd9-4776-880b-93b580f101ce)