feat: Convert event builder module to TS #680
Conversation
yavorona
changed the title
| }; | ||
| context: EventContext; | ||
| event: { | ||
| id: string | null; |
There was a problem hiding this comment.
No, it is not an optional property and can be of type string or null
There was a problem hiding this comment.
@keppel2 you are right by intuition but JS and and TS are weird languages. id? is equivalent to string | undefined but not equivalent to string | null.
| }; | ||
| revenue: number | null; | ||
| value: number | null; | ||
| tags: EventTags | undefined; |
There was a problem hiding this comment.
this can probably be tags?
There was a problem hiding this comment.
Not sure about this change. I think declaring this property as optional may not be the right way since we always pass tags to buildConversionEvent method. Its value can be of type EventTags or undefined if user does not specify it in track call. @zashraf1985 Please LMKWYT
There was a problem hiding this comment.
I think it makes sense. Ignore my comment on this
| const configObj = config.configObj; | ||
| const decisionObj = config.decisionObj; | ||
| const userId = config.userId; | ||
| const flagKey = config.flagKey; | ||
| const enabled = config.enabled; | ||
| const userAttributes = config.userAttributes; | ||
| const clientEngine = config.clientEngine; | ||
| const clientVersion = config.clientVersion; |
There was a problem hiding this comment.
| const configObj = config.configObj; | |
| const decisionObj = config.decisionObj; | |
| const userId = config.userId; | |
| const flagKey = config.flagKey; | |
| const enabled = config.enabled; | |
| const userAttributes = config.userAttributes; | |
| const clientEngine = config.clientEngine; | |
| const clientVersion = config.clientVersion; | |
| const { | |
| configObj, | |
| decisionObj, | |
| userId, | |
| flagKey, | |
| enabled, | |
| userAttributes, | |
| clientEngine, | |
| clientVersion, | |
| } = config; |
| * @param {ImpressionConfig} config | ||
| * @return {ImpressionEvent} an ImpressionEvent object | ||
| */ | ||
| export const buildImpressionEvent = function(config: ImpressionConfig): ImpressionEvent { |
There was a problem hiding this comment.
You can actually destructure right here something like
function({configObj, userId .... }: ImpressionConfig)
| * @param {ConversionConfig} config | ||
| * @return {ConversionEvent} a ConversionEvent object | ||
| */ | ||
| export const buildConversionEvent = function(config: ConversionConfig): ConversionEvent { |
There was a problem hiding this comment.
you can destructure config properties right here in the signature
| import { LoggerFacade } from '@optimizely/js-sdk-logging'; | ||
| import { EventV1 as CommonEventParams} from '@optimizely/js-sdk-event-processor'; |
There was a problem hiding this comment.
Probably personal preference but i prefer to keep module imports above the relative ones
| function getCommonEventParams(options: ImpressionOptions | ConversionEventOptions): CommonEventParams { | ||
| const attributes = options.attributes; | ||
| const configObj = options.configObj; | ||
| const anonymize_ip = configObj.anonymizeIP ? configObj.anonymizeIP : false; | ||
| const botFiltering = configObj.botFiltering; |
There was a problem hiding this comment.
Some destructuring can probably reduce some code here too
| const campaignId = experimentId ? getLayerId(configObj, experimentId) : null; | ||
|
|
||
| let variationKey = variationId ? getVariationKeyFromId(configObj, variationId) : null; | ||
| variationKey = variationKey ? variationKey: ''; |
There was a problem hiding this comment.
| variationKey = variationKey ? variationKey: ''; | |
| variationKey = variationKey || ''; |
yavorona
force-pushed
the
pnguen/convert-event-builder-to-ts
branch
from
ba217b0 to
1c39080
Compare
| const ruleType = decisionObj.decisionSource; | ||
| const experimentKey = decision.getExperimentKey(decisionObj); | ||
| const variationKey = decision.getVariationKey(decisionObj); | ||
|
|
There was a problem hiding this comment.
Super NIT: Extra line break
Summary
event_buildermodule to TSTest plan
Existing unit tests