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

Replace MSE SDK with direct access to ACDL #19

Open
wants to merge 17 commits into
base: elsie-base-design
Choose a base branch
from

Conversation

herzog31
Copy link
Member

@herzog31 herzog31 commented Mar 11, 2024

Description

  • Replace Magento Storefront Events SDK and instead write events directly into ACDL
  • For categories, the widget adds a categories query to retrieve category information.
    • This category information is required to set the categoryContext required for product recommendations.
    • To send this query, the category id is required which has to be provided via currentCategoryId.
    • When currentCategoryId is provided, it will also be used as filter for productSearch.
  • Allow to customize the Catalog Service / LiveSearch endpoint using the apiUrl configuration parameter.

Motivation and Context

  • Improve performance

How Has This Been Tested?

See hlxsites/aem-boilerplate-commerce#11

Types of changes

  • Bug fix (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 change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@MichaelHeinzman
Copy link

MichaelHeinzman commented Jul 17, 2024

Is this removing events for sensei? My understanding is that the MSE SDK is needed to send events to sensei. Is this possible through ACDL? ( I have a limited understanding here lol)

@herzog31
Copy link
Member Author

herzog31 commented Jul 17, 2024

@MichaelHeinzman MSE SDK is a wrapper around ACDL which introduces some overhead. ACDL itself is pending some improvements in this regard as well. For Edge Delivery storefronts in particular, we try to keep the implementation as lightweight as possible. From an eventing / Sensei perspective, this PR does not change or break any existing functionality.

@MichaelHeinzman
Copy link

Oh okay thank you, I'll look into it more then.

@herzog31 herzog31 changed the base branch from elsie-base-design to main August 21, 2024 12:53
@herzog31 herzog31 changed the base branch from main to elsie-base-design August 21, 2024 12:53
if (categorySearch) {
magentoStorefrontEvtPublish?.categoryResultsView &&
magentoStorefrontEvtPublish.categoryResultsView(SEARCH_UNIT_ID);
if (categorySearch && categoryId) {

Choose a reason for hiding this comment

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

@herzog31 Should this be an or condition instead? Everywhere else seems to have a fallback of category path first, then category id.
This is particularly an issue with Edge Delivery since the content is the same across all backend environments, and category IDs vary per environment. We would need to make an initial graphql call to get the category ID based on the category path and then initiate this widget

Copy link
Member Author

Choose a reason for hiding this comment

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

@justinconabree I think the and condition is required here as the categoryId is required for retrieving the full category data required for setting the category context. While the category path can still be used to display a PLP (compared to search results), it's not possible to use the category path alone to perform a categories GraphQL query to get the full data required for the storefront events.

Choose a reason for hiding this comment

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

@herzog31 Got it! What would the solution be then for an Edge implementation? Fetching the category ID before initializing the dropin? 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

For now we add the category id in the markup to pass it to the widget like here https://www.aemshop.net/gear.md.

If you have multiple environments which use different ids, you could introduce different content sources per environment. https://www.aem.live/docs/repoless could be helpful for creating multiple projects with dedicated content sources per environment with a shared code base.

Alternatively, as you suggested you can do another query to retrieve the category id.

Choose a reason for hiding this comment

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

@herzog31 to clarify, by environment I mean staging VS production. Normally we wouldn't setup a new project between environments.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I'm referring to. Normally it's sufficient to use the same content source and code base for all environments and rely on different config files as done in the boilerplate. If you have more complex content processes (different content between prod and stage), creating multiple repoless projects is one way to solve this.

Choose a reason for hiding this comment

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

@herzog31 this is what we do as well. Our configuration file for staging points to the sandbox Catalog Service (configured to use staging Commerce's data), and our production config file uses production Catalog Service and production Commerce data.
We'll consider setting up a project for each environment going forward, though this is not ideal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants