-
Notifications
You must be signed in to change notification settings - Fork 0
feat(Campaign): make context creation extensible via async overridable template method #315
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: timowestnosto <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot act on feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces an extensible createContext template method to the Campaign component, enabling subclasses to customize context data before template rendering while maintaining full backward compatibility.
- Adds a new
createContextmethod to theCampaignclass as an overridable extension point - Modifies the
loadCampaignfunction to use the instance method instead of callinggetContextdirectly - Includes comprehensive test coverage for the subclass override functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/components/Campaign/Campaign.ts | Adds the createContext template method and updates loadCampaign to use it |
| test/components/Campaign.spec.tsx | Adds test coverage for subclass override behavior with custom context properties |
… test Co-authored-by: timowestnosto <[email protected]>
src/components/Campaign/Campaign.ts
Outdated
| * } | ||
| * ``` | ||
| */ | ||
| createContext(raw: JSONResult): object { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what scenarios this will be useful? A custom campaign can only override context data?
Also, JSON recommendation results are of Nested objects. I am not sure if extending it at the top level is needed. Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that I need to make this asynchronous, but the intended use cases are
- add utility functions to context
- use Shopify side product data instead
Co-authored-by: timowestnosto <[email protected]>
| * class CustomCampaign extends Campaign { | ||
| * async createContext(raw) { | ||
| * const context = await super.createContext(raw); | ||
| * return { ...context, customProperty: 'value' }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still not understanding.
- add utility functions - Can you elaborate on what kind of utility functions we are planning to expose to, for e.g., merchant's template? Are we going to create these CustomCampaign elements case-by-case basis and expose utility functions specific to every case?
- Use Shopify product data - How/Why do we mix Shopify product data into Nosto's recommendation data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functions that could be useful are
- formatting functions
- event listener functions
We replace Nosto recommendation data with Shopify data for alignment
| * | ||
| * @example | ||
| * ```typescript | ||
| * class CustomCampaign extends Campaign { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the cases where we would extend the Campaign custom element? Wouldn't it make it complex trying to promote inheritance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see good alternatives for inheritance here, since registration of custom functionality into custom element classes is also not something that is available as a standard pattern
This PR introduces an extensible async
createContexttemplate method to theCampaigncomponent, allowing subclasses and future variations to customize context data before template rendering with support for asynchronous operations.Problem
Previously, the
Campaigncomponent calledgetContext(...)directly during template rendering, making it impossible for subclasses to extend or modify the context without duplicating the entire render logic. This prevented scenarios like:Solution
Added an
async createContext(raw: JSONResult): Promise<object>method to theCampaignclass that:awaitinstead of the directgetContextusage inloadCampaignPromise<object>return type for better type safetyUsage Example
Changes
createContextmethod toCampaignclass with detailed documentationloadCampaignfunction to useawaitwith the instance method instead of directgetContextcallPromise<object>return type for better type safetyThe implementation is minimal and surgical - only one line changed in the rendering logic to add
awaitand one async method added to the class.Fixes #314.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.