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.

Sign up for GitHub

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

add gpp enum to experience field #101

Closed
wants to merge 3 commits into from
Closed

add gpp enum to experience field #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ce50d5d
add gpp enum to experience field
Nov 2, 2023
ceb11b5
add to default experience
Nov 2, 2023
341bdd7
bump version
Nov 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"author": "Transcend Inc.",
"name": "@transcend-io/airgap.js-types",
"description": "TypeScript types for airgap.js interoperability with custom consent UIs",
"version": "10.7.0",
"version": "10.7.1",
"homepage": "https://github.com/transcend-io/airgap.js-types",
"repository": {
"type": "git",
Expand Down
10 changes: 10 additions & 0 deletions src/enums/experience.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,13 @@ export const RegionsOperator = makeEnum({
export type RegionsOperator =
typeof RegionsOperator[keyof typeof RegionsOperator];

/**
* Set of IAB signals that we can communicate
*/
export enum IABSignal {
GppUsp = 'GPP_USP',
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I suppose this will be extended to include TCF, other GPP signals?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was figuring start with GPP, but I could definitely imagine refactoring TCF to include this as well, yeah!

}

export interface ExperienceInput {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@csmccarthy csmccarthy Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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

/** The regime determining the default experience */
name: PrivacyRegime;
Expand All @@ -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[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we need this to be an array because a customer might need both USP and US CA, for instance?

}

/**
Expand Down Expand Up @@ -169,6 +178,7 @@ export function defaultExperience(regime: PrivacyRegime): ExperienceInput {
false,
}),
),
iabSignals: [],
};
}

Expand Down
Loading