Update ExP Service to have user account based filters #620
Conversation
| */ | ||
| readonly initializePromise: Promise<void>; | ||
| onDidTreatmentsChange: Event<void>; |
There was a problem hiding this comment.
Looks like this event is not used anywhere and it might not be true that treatments have changed on refresh, so this event will be triggered falsely.
There was a problem hiding this comment.
@sandy081 It is true, the exp package doesn't actually tell us this information, so in order to be really accurate we'd have to keep a list of every queried treatment and a cache of its value and if they are not equivalent, we could fire a more accurate event. However, I don't know how valuable it will be, most consumers can just check the value for free when the event signals and decide if something should happen. The primary consumer of this would be the configuration service, which I tried hooking up, but got into a dependency cycle. Since we don't do that refresh today, I'd like to take that as a second PR.
There was a problem hiding this comment.
Ok. That is the reason I thought having this change event is misleading. In VS Code core, I ask for settings related experiments every 60 minutes instead of adding a change event in experiment service.
So, I suggest to do the same here.
There was a problem hiding this comment.
Although config service will be the primary consumer, I won't necessarily be the only consumer as some ExPs don't have settings. Also, we have a separate consideration now introduced in the PR. Your authentication state can cause the treatments to change, not just upon a general refresh. I plan to work with you to bring this behavior to core as well.
There was a problem hiding this comment.
But I would not prefer to use the change event if it is not sure about the values being changed or not.
This PR also greatly reduces the service API to simplify things. We also refresh treatments every 1 hour.
@sandy081 I'd love to get your feedback as this is related to the work you did in core and it affects configurations.