Implement JS Build (ES6) and JS Lint #261

puntope · 2023-03-23T14:31:07Z

Changes proposed in this Pull Request:

Closes part of #227 .

This PR implements the next major changes.

Add wp scripts and a basic webpack config for compiling purposes.
source files are now in assets/js/src and the build in assets/js/build
Exclude source files assets/js/src to be included in archived zip file
Add js lint support as well as a github workflow for checking so on each PR
Copy the Blocks actions present in Google Analytics Integration - Action Hooks and initial tracking events woocommerce-blocks#3953

Checks:

Does your code follow the WordPress coding standards?
Have you written new tests for your changes, as applicable?
Have you successfully run tests with your changes locally?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Screenshots:

Detailed test instructions:

Run npm i
Run npm run start
Check the extension loads the ga-integration.js file ga-admin-settings.js file and they work correctly without errors.
Go to one of the files in assets/js/src and add a random console.log
See how the re-build process runs
Refresh the page and see the console log running
Check the es-lint workflow is working fine
Try to make some bad changes like undefined constants or bad spacing and see the js linter working.
run npm run archive
See the zip file doesn't contain anymore asset/js/src but yes asset/js/build
Verify the config files

Additional details:

⚠️ JS Unit Tests support are not included in this PR
⚠️ A solution for dealing with duplicate actions with blocks will be done in the future PR
⚠️ To discuss if we want to add TypeScript on this project. Seems like a good moment and extension to do so.

Changelog entry

Dev - Implement JS Build (ES6) and JS Lint

eason9487

Thanks for the work.

Unfortunately, I was hardly able to complete the test.

Found some critical issues:

The assets/js/src/ga-integration.js script is not loaded to the page. And even if after fixing the loading issue, the "google_analytics_integration_product_data is not defined" errors will occur.
All the event tracking in assets/js/src/actions.js are not fired since the mismatched hook names. Further, even if fix the hook names, "Cannot read properties of undefined (reading 'price')" errors will occur when calling the trackAddToCart function.
The event tracking in WC Blocks has changed quite a lot, for example, the number of events has increased from 4 to 16. Does it need to sync the subsequent changes?

Other issues that are also worth noting:

Some project configurations could be omitted or enhanced.
Two scripts' $deps array is not set properly.

assets/js/src/ga-integration.js

.eslintrc.js

eason9487 · 2023-03-23T15:43:56Z

includes/class-wc-google-analytics.php

@@ -377,7 +377,7 @@ public function load_admin_assets() {
 			return;
 		}

-		wp_enqueue_script( 'wc-google-analytics-admin-enhanced-settings', plugins_url( '/assets/js/admin-ga-settings.js', dirname( __FILE__ ) ), array(), WC_GOOGLE_ANALYTICS_INTEGRATION_VERSION, true );
+		wp_enqueue_script( 'wc-google-analytics-admin-enhanced-settings', plugins_url( '/assets/js/build/admin-ga-settings.js', dirname( __FILE__ ) ), array(), WC_GOOGLE_ANALYTICS_INTEGRATION_VERSION, true );


There is an original issue but maybe consider fixing it by the way.

The admin-ga-settings.js script depends on jQuery so it should have 'jquery' handle in the $deps array.

Should be resolved here f3e783b

Plugin::get_instance()->get_js_asset_dependencies( 'admin-ga-settings' ) returns an empty array. The 'jquery' handle is still missing.

includes/class-wc-google-gtag-js.php

.eslintrc.js

assets/js/src/constants.js

assets/js/src/actions.js

assets/js/src/utils.js

Co-authored-by: Eason <[email protected]>

puntope · 2023-04-03T19:15:17Z

Howdy @eason9487 first of all, thanks for the amazing review as always.

I patched all of your findings. Still need to test deeper to see everything is working find. But I think at least we can have another round to check most of the issues :)

The event tracking in WC Blocks has changed quite a lot, for example, the number of events has increased from 4 to 16. Does it need to sync the subsequent changes?

Oh, I see. Maybe we can keep this one like it is (with the reduced amount) without merge and make those in a future PR if thats ok.

package.json

assets/js/src/actions.js

eason9487

The event tracking in WC Blocks has changed quite a lot, for example, the number of events has increased from 4 to 16. Does it need to sync the subsequent changes?

Oh, I see. Maybe we can keep this one like it is (with the reduced amount) without merge and make those in a future PR if thats ok.

Keeping it as it's would be OK but I guess it would need a feature flag to skip the wp_enqueue_script call by default for the actions.js file, or even remove the whole wp_enqueue_script call for now. So that it won't lead to duplicate event tracking with WC Blocks.

package.json

eason9487 · 2023-04-05T11:07:50Z

includes/class-wc-google-analytics.php

@@ -377,7 +377,7 @@ public function load_admin_assets() {
 			return;
 		}

-		wp_enqueue_script( 'wc-google-analytics-admin-enhanced-settings', plugins_url( '/assets/js/admin-ga-settings.js', dirname( __FILE__ ) ), array(), WC_GOOGLE_ANALYTICS_INTEGRATION_VERSION, true );
+		wp_enqueue_script( 'wc-google-analytics-admin-enhanced-settings', plugins_url( '/assets/js/build/admin-ga-settings.js', dirname( __FILE__ ) ), array(), WC_GOOGLE_ANALYTICS_INTEGRATION_VERSION, true );


Plugin::get_instance()->get_js_asset_dependencies( 'admin-ga-settings' ) returns an empty array. The 'jquery' handle is still missing.

includes/class-wc-google-gtag-js.php

woocommerce-google-analytics-integration.php

puntope · 2023-04-06T12:49:41Z

The event tracking in WC Blocks has changed quite a lot, for example, the number of events has increased from 4 to 16. Does it need to sync the subsequent changes?

Oh, I see. Maybe we can keep this one like it is (with the reduced amount) without merge and make those in a future PR if thats ok.

Keeping it as it's would be OK but I guess it would need a feature flag to skip the wp_enqueue_script call by default for the actions.js file, or even remove the whole wp_enqueue_script call for now. So that it won't lead to duplicate event tracking with WC Blocks.

I created the feature feature/analytics-wc-blocks so we can safely merge everything there instead of trunk.

puntope · 2023-04-06T12:52:23Z

Howdy @eason, I added new patches and some question in regards your comment here #261 (comment)

eason9487

Thanks for the further adjustment. LGTM.

Implement JS Build (ES6) and JS Lint

23d9af4

puntope self-assigned this Mar 23, 2023

puntope requested a review from a team March 23, 2023 14:36

puntope marked this pull request as ready for review March 23, 2023 14:37

eason9487 requested changes Mar 24, 2023

View reviewed changes

puntope and others added 13 commits April 3, 2023 15:20

Add Jquery as a global and remove unnevesary lint exception

f61ea20

Add missing prettier and remove uglify

da1284f

Exclude eslintrc.js

67d7e89

Remove unnecesary directives in eslintr file

ad25ad9

Refactor admin load assets

f3e783b

Make google_analytics_integration_product_data globally accesible

be57224

Fix translation domain

108a856

Change NAMESPACE for the actions hook

d30bce6

Fix destructuring bug

890cee5

Update assets/js/src/actions.js

824ae0f

Co-authored-by: Eason <[email protected]>

Restore original implementation for get product id function

38214ee

ES LInt

f2063fb

PHPCS

12800af

eason9487 reviewed Apr 5, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

assets/js/src/actions.js Outdated Show resolved Hide resolved

eason9487 reviewed Apr 5, 2023

View reviewed changes

puntope added 4 commits April 6, 2023 16:07

Add extra dependencies

450963e

Remove eslint-import-resolver-webpack dependency

3b5ea43

Update comments

f2689e1

PHPCS

3a587d8

puntope requested a review from eason9487 April 6, 2023 12:47

puntope changed the base branch from trunk to feature/analytics-wc-blocks April 6, 2023 12:47

Remove composer install from scripts

33e19c8

eason9487 approved these changes Apr 7, 2023

View reviewed changes

puntope merged commit 025214b into feature/analytics-wc-blocks Apr 10, 2023

puntope deleted the feature/ga-actions branch April 10, 2023 10:58

eason9487 added the changelog: dev Developer-facing only change. label May 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement JS Build (ES6) and JS Lint #261

Implement JS Build (ES6) and JS Lint #261

puntope commented Mar 23, 2023

eason9487 left a comment •

edited

Loading

eason9487 Mar 23, 2023

puntope Apr 3, 2023

eason9487 Apr 5, 2023

puntope commented Apr 3, 2023

eason9487 left a comment

eason9487 Apr 5, 2023

puntope commented Apr 6, 2023

puntope commented Apr 6, 2023

eason9487 left a comment

Implement JS Build (ES6) and JS Lint #261

Implement JS Build (ES6) and JS Lint #261

Conversation

puntope commented Mar 23, 2023

Changes proposed in this Pull Request:

Checks:

Screenshots:

Detailed test instructions:

Additional details:

Changelog entry

eason9487 left a comment • edited Loading

Choose a reason for hiding this comment

eason9487 Mar 23, 2023

Choose a reason for hiding this comment

puntope Apr 3, 2023

Choose a reason for hiding this comment

eason9487 Apr 5, 2023

Choose a reason for hiding this comment

puntope commented Apr 3, 2023

eason9487 left a comment

Choose a reason for hiding this comment

eason9487 Apr 5, 2023

Choose a reason for hiding this comment

puntope commented Apr 6, 2023

puntope commented Apr 6, 2023

eason9487 left a comment

Choose a reason for hiding this comment

eason9487 left a comment •

edited

Loading