add gpp enum to experience field #101
Conversation
|
This pull request has been linked to 1 task:
|
There was a problem hiding this comment.
LGTM, but I think I'd like @anotherminh 's and/or @eligrey 's eyes on this to make sure this makes sense.
| * Set of IAB signals that we can communicate | ||
| */ | ||
| export enum IABSignal { | ||
| GppUsp = 'GPP_USP', |
There was a problem hiding this comment.
Interesting. I suppose this will be extended to include TCF, other GPP signals?
There was a problem hiding this comment.
I was figuring start with GPP, but I could definitely imagine refactoring TCF to include this as well, yeah!
| @@ -138,6 +145,8 @@ export interface ExperienceInput { | |||
| viewState: InitialViewState; | |||
| /** experience purposes to be added */ | |||
| experiencePurposeInputs: ExperiencePurposeInput[]; | |||
| /** set of IAB signals to be communicated in this experience */ | |||
| iabSignals: IABSignal[]; | |||
There was a problem hiding this comment.
I suppose we need this to be an array because a customer might need both USP and US CA, for instance?
| export enum IABSignal { | ||
| GppUsp = 'GPP_USP', | ||
| } | ||
|
|
||
| export interface ExperienceInput { |
There was a problem hiding this comment.
It doesn't look like this is used anywhere outside of being the return type for this function? @csmccarthy can you explain a bit about the overall larger goal here? It seems like we'd want to add a new config option, iabSignals, which is really just a config option for the gpp module if that is loaded, correct?
If so, I'm not sure this is the right place for it. In general, I'm not sure if we have a good pattern in place right now for encoding module-specific configuration -- essentially, additional "parameters" that we would like to pass to the module. @eligrey i think has thoughts on this...
If we did want to expose this module-specific configuration to airgap, then it would live here https://github.com/transcend-io/main/blob/e78a86bb6c3556c64d554e43f37898d4f2af09cb/frontend-support/ag-types/src/core.ts#L831-L842
There was a problem hiding this comment.
I think this makes sense to be exposed as an arbitrary rich config input field for module descriptors. The module itself could consume the input through a well-defined interface.
I've thought about this a lot in the past when it comes to ag-core & XDI (e.g. could ag-core share its copy of nrc with xdi). There is no clean place to put this module input that works the same for both ESM and mon-ESM.
I'm going to think about it for a bit and come back with a unified model to solve this. Both ESM and non-ESM would get their own unique config reading process
There was a problem hiding this comment.
@anotherminh You've got the gist of it, yeah: the point was to define an enum whose values get loaded into the loadOptions so that the module has an idea of what to activate.
Broader scale, it's just a way for information to pass through from the experience record to the module that experience designates for loading, and there could very well be a better way to do that. I was following the tcfUI example here, which defines a type in airgap.js-types then uses that to define the loadOptions type, but I know that module is exceptional in a lot of ways so if there's a better way for me to do it I'm all for it.
I think editing the ModuleDescriptors is an interesting idea, but it seems to me like since those descriptors aren't (afaik) exposed globally, it would require airgap have the ability interface more strongly with the modules it's loading, e.g. through a function call or something, or otherwise expose those descriptors for the modules to retrieve those parameters themselves. I don't really have an opinion here on which would be better, but it seems like the latter is closer to how it works today so it might be easier to transition to that
There was a problem hiding this comment.
@csmccarthy do you m ind also reorganizing this file? enums.ts should not contain anything other than enums.... the interface + helper function should live in its own file
Related Issues
Internal Changelog
This is priming the type system to allow us to add sets of iab signals to the experiences object. I was a bit torn between where our source of truth for this enum was going to live - here vs cm-types vs ag-types - but ultimately I think it fits in well here since its main purpose will be to inform the new GPP load option. If you'd suggest somewhere else lmk