-
Notifications
You must be signed in to change notification settings - Fork 74
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.
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
feat: streaming and polling deltas #712
Conversation
@@ -401,13 +439,18 @@ Message: ${err.message}`, | |||
} else if (res.ok) { | |||
nextFetch = this.countSuccess(); | |||
try { | |||
const data: ClientFeaturesResponse = await res.json(); | |||
const data = await res.json(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can be either full response type or delta type
|
||
export const SUPPORTED_SPEC_VERSION = '4.3.0'; | ||
export const SUPPORTED_SPEC_VERSION = '5.2.0'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this hasn't been updated for a while
} else { | ||
this.initialEventSourceConnected = true; | ||
} | ||
this.handleFlagsFromStream(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in streaming mode we always rely 100% on streaming. previously first request in streaming was a polling request
@@ -97,7 +105,7 @@ export default class Repository extends EventEmitter implements EventEmitter { | |||
|
|||
private eventSource: EventSource | undefined; | |||
|
|||
private initialEventSourceConnected: boolean = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to check if first connection is different from another since all streaming goes with SSEs
} catch (err) { | ||
this.emit(UnleashEvents.Error, err); | ||
} | ||
} | ||
|
||
timedFetch(interval: number) { | ||
if (interval > 0 && !this.eventSource) { | ||
if (interval > 0 && this.mode.type === 'polling') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we added explicit mode in the constructor params since we need more than eventSource (mode + format)
@@ -192,7 +197,11 @@ export default class Repository extends EventEmitter implements EventEmitter { | |||
|
|||
async start(): Promise<void> { | |||
// the first fetch is used as a fallback even when streaming is enabled | |||
await Promise.all([this.fetch(), this.loadBackup(), this.loadBootstrap()]); | |||
await Promise.all([ | |||
this.mode.type === 'streaming' ? Promise.resolve() : this.fetch(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to fetch data on startup with HTTP call when streaming is enabled
}); | ||
|
||
this.setReady(); | ||
this.emit(UnleashEvents.Changed, Object.values(this.data)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decided not to introduce new event in this PR. We may consider ChangedDelta later on
@@ -379,7 +417,7 @@ Message: ${err.message}`, | |||
if (this.tags) { | |||
mergedTags = this.mergeTagsToStringArray(this.tags); | |||
} | |||
const url = getUrl(this.url, this.projectName, this.namePrefix, mergedTags); | |||
const url = getUrl(this.url, this.projectName, this.namePrefix, mergedTags, this.mode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetch call need to be conditional
): string => { | ||
const url = resolveUrl(base, './client/features'); | ||
const isDeltaPolling = mode && mode.type === 'polling' && mode.format === 'delta'; | ||
const url = resolveUrl(base, isDeltaPolling ? './client/delta' : './client/features'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delta has to be opt in
@@ -76,7 +76,7 @@ export class Unleash extends EventEmitter { | |||
storageProvider, | |||
disableAutoStart = false, | |||
skipInstanceCountWarning = false, | |||
experimentalMode = { type: 'polling' }, | |||
experimentalMode = { type: 'polling', format: 'full' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by default keep doing what we have been doing so far
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
About the changes
Delta support for streaming and polling.
Our Mode type shows what options are possible
Mode = { type: 'polling'; format: 'delta' | 'full' } | { type: 'streaming' };
. In streaming you have no choice and deltas is your only options. In polling mode you can choose and we default to full format.Important files
Discussion points