Skip to content
This repository was archived by the owner on Jan 30, 2025. It is now read-only.

Conversation

@circlecube
Copy link
Member

@circlecube circlecube commented Jan 14, 2025

Proposed changes

This changes how the e-commerce module components are loaded in the plugin app. Before they were loaded via a package, but this updates it to directly load them in the plugin app like we do for other modules. We load the js components directly from the module in the vendor directory. This will need considerable testing on live sites.

This would eliminate the need for a js build package in the e-commerce module.

We'd also want to restructure the module and move the components out of the src directory and into a top-level components dir (src directories are usually excluded from builds). This effort is to bring all modules to a more consistent architecture.

Open Discussion

Is there a reason I'm missing that the build step is still required for the e-commerce module components? Removing it would simplify the plugin release process and make more of the modules lend components to the plugin rather than require a composer and npm package to run.

Type of Change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • Linting and tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@cypress
Copy link

cypress bot commented Jan 14, 2025

Bluehost Brand Plugin    Run #12798

Run Properties:  status check passed Passed #12798  •  git commit 2364fb65e2: Merge branch 'develop' into try/loading-ecommerce-module-components-directly
Project Bluehost Brand Plugin
Branch Review try/loading-ecommerce-module-components-directly
Run status status check passed Passed #12798
Run duration 32m 16s
Commit git commit 2364fb65e2: Merge branch 'develop' into try/loading-ecommerce-module-components-directly
Committer Evan Mullins
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 33
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 407
View all changes introduced in this branch ↗︎

@cypress
Copy link

cypress bot commented Jan 14, 2025

Bluehost Brand Plugin    Run #12800

Run Properties:  status check failed Failed #12800  •  git commit 2844552025 ℹ️: Merge 2364fb65e2e348fd1cbc553b3d5c705e83e05d64 into 6d0a00497d14bc2537251706b72b...
Project Bluehost Brand Plugin
Branch Review try/loading-ecommerce-module-components-directly
Run status status check failed Failed #12800
Run duration 35m 18s
Commit git commit 2844552025 ℹ️: Merge 2364fb65e2e348fd1cbc553b3d5c705e83e05d64 into 6d0a00497d14bc2537251706b72b...
Committer Evan Mullins
View all properties for this run ↗︎

Test results
Tests that failed  Failures 1
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 33
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 406
View all changes introduced in this branch ↗︎

Tests for review

Failed  vendor/newfold-labs/wp-module-solutions/tests/cypress/integration/solutions-app.cy.js • 1 failed test

View Output Video

Test Artifacts
My Plugins and Tools in Plugin App > My Plugins & Tools displays with Solution Test Replay Screenshots Video
Flakiness  vendor/newfold-labs/wp-module-coming-soon/tests/cypress/integration/coming-soon-woo.cy.js • 1 flaky test

View Output Video

Test Artifacts
Coming Soon with WooCommerce > Replace our admin bar site status badge with WooCommerce's when active Test Replay Screenshots Video
Flakiness  vendor/newfold-labs/wp-module-onboarding/tests/cypress/integration/2-general-onboarding-flow/basic-info.cy.js • 1 flaky test

View Output Video

Test Artifacts
Basic Info Page > Check Drawer Activity Test Replay Screenshots Video
Flakiness  vendor/newfold-labs/wp-module-onboarding/tests/cypress/integration/3-ecommerce-onboarding-flow/basic-info.cy.js • 1 flaky test

View Output Video

Test Artifacts
Basic Info Page > Check Drawer Activity Test Replay Screenshots Video

@wpalani
Copy link
Member

wpalani commented Jan 14, 2025

  • I'm all for this. All of the other modules are imported from vendor.
  • I'm not aware of a specific reason why it's shipped as a GH/NPM package.
  • And yes, we'll need to test this thoroughly on Prod sites before merging.

@circlecube
Copy link
Member Author

This feels especially doable since the ecommerce module already loads the build file on the plugin app screen. See: https://github.com/newfold-labs/wp-module-ecommerce/blob/trunk/includes/ECommerce.php#L372-L383

The index is currently ignored in git, but I'm adding it back now. This should make things pretty easy to sidestep needing an e-commerce package at all. 🎉

…nents-directly

* release/3.16.0: (23 commits)
  update to latest ecommerce and facebook modules
  revert to wp-scripts 27 and remove react-jsx-runtime polyfill
  adjust polyfill path and remove from normal webpack config
  happy lint happy life
  add jsx-runtime-polyfill for wp6.5 support - remove unused package
  bump installer version in composer to match lock
  update htaccess-manager package to fix 7.3 issue in php
  add missing package according to lint
  update version to 3.16.0
  Composer(deps): Bump newfold-labs/wp-module-onboarding
  NPM Dev(deps-dev): Bump @tailwindcss/forms from 0.5.9 to 0.5.10
  NPM(deps): Bump @wordpress/icons from 10.15.1 to 10.16.0
  NPM(deps): Bump @wordpress/dom-ready from 4.14.0 to 4.16.0
  Composer(deps): Bump newfold-labs/wp-module-coming-soon
  update remaining modules for runtime updates
  use latest fully released packages
  update to latest runtime and ecommerce module
  add @newfold packaged to tailwind config
  NPM Dev(deps-dev): Bump @wordpress/env from 10.14.0 to 10.16.0
  update to runtime 1.1.3 release
  ...

# Conflicts:
#	package-lock.json
#	package.json
#	src/app/pages/ecommerce/page.js
#	src/app/pages/home/freeAddonsSection.js
#	src/app/pages/home/index.js
#	src/app/pages/home/welcomeSection.js
@circlecube circlecube self-assigned this Jan 22, 2025
…irectly

* develop:
  allow cypress matrix 60 min
…irectly

* develop:
  NPM(deps): Bump react-router-dom from 7.1.1 to 7.1.3

# Conflicts:
#	package-lock.json
@circlecube circlecube marked this pull request as ready for review January 22, 2025 16:57
@circlecube
Copy link
Member Author

Before merging this we may want to have a larger discussion about best practices and module architecture. If we want to update this one to be more in line with the others, we should make sure we're following a best practice.

I've also had discussions about updating the plugin app to use slots and slotfills for modules to add components into assigned places, but I think we'd need some considerable updates to get to a place we can do this.

@circlecube
Copy link
Member Author

Will take care of this update in the new repository at https://github.com/newfold-labs/wp-plugin-bluehost

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants