Skip to content

Conversation

rafinutshaw-optimizely
Copy link
Contributor

Summary

  • Add fetchqualifiedsegments options params to onReady client method
  • Fetch qualified segments in onready if usercontext is available

Test plan

  • fetchqualifiedsegment method is called in onReady

Issues

Copy link
Contributor

@mikechu-optimizely mikechu-optimizely left a comment

Choose a reason for hiding this comment

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

LGTM, with no showstoppers from my perspective. I have a couple of questions below.

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

Questions about fetch timing.

Copy link
Contributor

@mikechu-optimizely mikechu-optimizely left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

The flag looks good. A couple of more questions.

reason: 'TIMEOUT',
message:
'failed to initialize onReady before timeout, either the datafile or user info was not set before the timeout',
'failed to initialize onReady before timeout, either the datafile, user info or qualified segments was not set before the timeout',
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this timeout is covering datafile ready timeout, not covering "qualifed segments" failure. The original message makes more sense.

return Promise.race([this.dataReadyPromise, timeoutPromise]).then(res => {
return Promise.race([this.dataReadyPromise, timeoutPromise]).then(async res => {
if (res.success) {
await this.fetchQualifiedSegments(config?.segmentOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if fetch fails with network error, timeout, etc? res should be updated accordingly?

@rafinutshaw-optimizely
Copy link
Contributor Author

Closing this due to change in requirement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants