Add event-processor package #239
Conversation
|
|
||
| layer: { | ||
| id: string | ||
| } | null |
There was a problem hiding this comment.
This shouldn't be null ever really.
Do we want to use campaign here?
| key: event.key, | ||
| }, | ||
|
|
||
| revenue: getRevenueValue(eventTags), |
There was a problem hiding this comment.
Should we just let strings pass through here?
There was a problem hiding this comment.
use regex to verify that it's a number only.
always send as a string
| [key: string]: string | number | null | ||
| } | ||
|
|
||
| export function buildConversionEvent(opts: { |
There was a problem hiding this comment.
this appears to be unused in the integration of event-processor
There was a problem hiding this comment.
move all of these functions out to decision-service for now
| ) | ||
| return null | ||
| } | ||
| return parsed |
There was a problem hiding this comment.
Dont return the parsed value here, return the unparsed (could be string)
| ) | ||
| return null | ||
| } | ||
| return parsed |
There was a problem hiding this comment.
same as above, return unparsed.
| }) { | ||
| this.dispatcher = dispatcher | ||
|
|
||
| const factory = eventQueueFactory || new DefaultEventQueueFactory() |
There was a problem hiding this comment.
do we need the queue to be replaceable
| } | ||
|
|
||
| enqueue(event: K): void { | ||
| this.buffer.push(event) |
There was a problem hiding this comment.
start the timer only after the first event is put in
| // loop and apply all transformers | ||
| for (let transformer of this.transformers) { | ||
| try { | ||
| await transformer(event, projectConfig) |
There was a problem hiding this comment.
Should we give them a wrapper / interface to do mutation on here
What are the things they'd want to change:
- set user attribute
- set event
There was a problem hiding this comment.
LGTM. Really great code. Left some comments to consider, but I approve regardless.
- I don't understand why you want to add the models package in this PR. It's substantial amount of code that, from what I can tell, isn't even necessary right now (only used by transformers and interceptors, which are placeholders and not exposed for real use). I would suggest removing the package (or just keeping only
Managedin it), then we can add it and review it separately. But I still tried to give models a thorough review anyway. - In general I though it was best to avoid using the
anytype, unless there was truly no other option, but if you don't agree I'd like to understand why.
|
|
||
| describe('eventQueue', () => { | ||
| beforeEach(() => { | ||
| jest.useFakeTimers() |
There was a problem hiding this comment.
Weird, I would have thought you want to call jest.useRealTimers in afterEach to restore real timers. Does resetAllMocks restore real timers?
There was a problem hiding this comment.
Ah good catch. It's not actually using the real timers ever here.
For the test it doesn't really need to use real timers.
I'm not sure how jest works between running different files (if they are run in the same global context). But i'll add a useRealTimers in afterEach here.
| queue.stop() | ||
| }) | ||
|
|
||
| it('should flush the queue and invoke clearInterval', () => { |
There was a problem hiding this comment.
Nit: this test description doesn't seem to match what the test is doing.
| localCallback(true) | ||
| }) | ||
|
|
||
| it('should return a promise that is resolved when the dispatcher callback fires false', done => { |
There was a problem hiding this comment.
I'm confused, this test seems very similar to the previous one. What's the difference when the dispatcher fires false instead of true?
There was a problem hiding this comment.
this is testing the processor.stop() functionality.
This test is saying that even if the request fails to send but the dispatcher yielded control back, then the .stop() promise should be resolved
| event: EventV1, | ||
| } | ||
|
|
||
| export interface HttpEventDispatcher extends EventDispatcher { |
There was a problem hiding this comment.
Why extends EventDispatcher?
There was a problem hiding this comment.
It's ensuring this interface defined in HttpEventDispatcher is still a valid EventDispatcher
| export type EventTransformer = ( | ||
| event: ProcessableEvents, | ||
| projectConfig: ProjectConfig, | ||
| ) => Promise<any> |
There was a problem hiding this comment.
Should it be Promise<void> instead of Promise<any>?
There was a problem hiding this comment.
Probably should.
Once it's compiled this wont be enforced at all, but in TypeScript land you probably shouldn't be returning anything here.
| }) | ||
| } | ||
|
|
||
| start(): void { |
There was a problem hiding this comment.
Is having the start method on the EventQueue type pointless?
There was a problem hiding this comment.
It implements Managed. Yes in this instance having a start method is pointless. But for us to use the type Managed and have other parts of the system only know about Managed it needs to implement a no-op
packages/models/src/models.ts
Outdated
| [userId: string]: string // variationId | ||
| } | ||
|
|
||
| type ExperimentStatus = 'Running' | 'Launched' | 'Paused' | 'Not started' |
There was a problem hiding this comment.
When would you use a union of strings like this, vs. an enum?
There was a problem hiding this comment.
I dont have a great answer to this, they both have drawbacks.
An enum is probably better here, as it's easier to not make a typing mistake when comparing it
Remove unused events.ts methods out Switch out the queue implementation based maxQueueSize > 1 Dont make queue configurable
jordangarcia
force-pushed
the
jordan/add-event-processor-package
branch
from
f86000d to
74cc1bd
Compare
jordangarcia
changed the title
Summary
js-sdk-event-processorpackages to the main repo.Integration into the
optimizely-sdkpackage will occur in a later PR.Test plan