-
Notifications
You must be signed in to change notification settings - Fork 70
feat(ofrep): Disable polling by default #1510
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
ec25844
7ccc33c
63dfe92
1a61925
0cdb0d8
de2c49f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,7 @@ import type { FlagCache, MetadataCache } from './model/in-memory-cache'; | |
| import type { OFREPWebProviderOptions } from './model/ofrep-web-provider-options'; | ||
|
|
||
| export class OFREPWebProvider implements Provider { | ||
| DEFAULT_POLL_INTERVAL = 30000; | ||
| DEFAULT_POLL_INTERVAL = 0; | ||
|
|
||
| readonly metadata = { | ||
| name: 'OpenFeature Remote Evaluation Protocol Web Provider', | ||
|
|
@@ -57,6 +57,7 @@ export class OFREPWebProvider implements Provider { | |
| private _flagSetMetadataCache?: MetadataCache; | ||
| private _context: EvaluationContext | undefined; | ||
| private _pollingIntervalId?: number; | ||
| private _visibilityChangeHandler = this._onVisibilityChange.bind(this); | ||
|
|
||
| constructor(options: OFREPWebProviderOptions, logger?: Logger) { | ||
| this._options = options; | ||
|
|
@@ -86,6 +87,11 @@ export class OFREPWebProvider implements Provider { | |
| this.startPolling(); | ||
| } | ||
|
|
||
| // Listen for page/app visibility changes to refetch flags when becoming visible | ||
| if (typeof document !== 'undefined') { | ||
| document.addEventListener('visibilitychange', this._visibilityChangeHandler); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel this should be opt-in, or at least opt-out? Some folks may well want to load flags once on app start and ensure they don't change again?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I imagine this will be especially true in micro-frontend architectures where there may be many providers defined in different domains -- if they all refresh every time the page visiblity changes, that is a lot of network requests being fired off at once.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lukas-reining @MattIPv4 thanks for the suggestion, I added a config option for opt-in 👍
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you :) |
||
|
|
||
| this._logger?.debug(`${this.metadata.name} initialized successfully`); | ||
| } catch (error) { | ||
| if (error instanceof OFREPApiUnauthorizedError || error instanceof OFREPForbiddenError) { | ||
|
|
@@ -166,6 +172,9 @@ export class OFREPWebProvider implements Provider { | |
| */ | ||
| onClose?(): Promise<void> { | ||
| this.stopPolling(); | ||
| if (typeof document !== 'undefined') { | ||
| document.removeEventListener('visibilitychange', this._visibilityChangeHandler); | ||
| } | ||
| this._ofrepAPI.close(); | ||
| return Promise.resolve(); | ||
| } | ||
|
|
@@ -319,4 +328,30 @@ export class OFREPWebProvider implements Provider { | |
| clearInterval(this._pollingIntervalId); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Handler for visibility changes (page/app becoming visible) | ||
| * Re-fetches flags when the document becomes visible | ||
| * @private | ||
| */ | ||
| private async _onVisibilityChange() { | ||
| // Only re-fetch when the page becomes visible, not when it's hidden | ||
| if (typeof document !== 'undefined' && document.visibilityState === 'visible') { | ||
| try { | ||
| const now = new Date(); | ||
| if (this._retryPollingAfter !== undefined && this._retryPollingAfter > now) { | ||
| return; | ||
| } | ||
| const res = await this._fetchFlags(this._context); | ||
| if (res.status === BulkEvaluationStatus.SUCCESS_WITH_CHANGES) { | ||
| this.events?.emit(ClientProviderEvents.ConfigurationChanged, { | ||
| message: 'Flags updated after visibility change', | ||
| flagsChanged: res.flags, | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| this.events?.emit(ClientProviderEvents.Stale, { message: `Error while fetching after visibility change: ${error}` }); | ||
| } | ||
| } | ||
| } | ||
|
marcozabel marked this conversation as resolved.
Outdated
marcozabel marked this conversation as resolved.
Outdated
|
||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.