feat(ofrep): Disable polling by default#1510
feat(ofrep): Disable polling by default#1510marcozabel wants to merge 3 commits intoopen-feature:mainfrom
Conversation
- Change DEFAULT_POLL_INTERVAL from 30000ms to 0 (disabled by default) - Update OFREPWebProviderOptions documentation to reflect polling is now opt-in - Polling must be explicitly enabled by setting pollInterval to a positive number - This is a breaking change, appropriate for sub-v1 OFREP providers Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
- Change DEFAULT_POLL_INTERVAL from 30000ms to 0 (polling disabled by default) - Add visibility change detection to re-fetch flags when page/app becomes visible - Add _visibilityChangeHandler property and _onVisibilityChange() method - Register/unregister visibility change listener in initialize/onClose - Re-fetches respect Retry-After rate limit headers - Emits ConfigurationChanged or Stale events based on fetch results Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
marcozabel
force-pushed
the
feat/disable-polling-by-default
branch
from
5dacf91 to
7ccc33c
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a breaking change by disabling polling by default (setting pollInterval to 0) and adds a new feature that automatically re-fetches flags when the page or application becomes visible. Feedback suggests refactoring the flag-fetching logic into a shared helper method to reduce duplication and implementing a concurrency lock or flag in the visibility change handler to prevent redundant, overlapping network requests during rapid state transitions.
| 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}` }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for fetching flags and emitting events is duplicated between startPolling and _onVisibilityChange. Consider extracting this into a private helper method (e.g., _refreshFlags(reason: string)) to improve maintainability and ensure consistent error handling across different background update triggers.
| 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}` }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The _onVisibilityChange handler is an async function used as an event listener. Since visibilitychange can fire multiple times in quick succession (e.g., rapid tab switching), and _fetchFlags involves an asynchronous network request, there is a risk of multiple concurrent fetches. While the use of ETags mitigates server load, it's generally better to prevent overlapping requests by using a simple 'isFetching' flag or a concurrency lock to avoid race conditions and redundant network activity.
- Extract duplicated fetch+emit logic from startPolling and _onVisibilityChange into a shared _refreshFlags(reason) method - Add _isFetching flag to prevent overlapping concurrent requests from rapid visibility changes or polling - Addresses PR review feedback Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
| * Default: 30000 | ||
| * If a negative number or 0 is provided, the provider will not poll the OFREP API. | ||
| * This is the default behavior. Polling is available as an opt-in configuration. | ||
| * Default: 0 (disabled) |
There was a problem hiding this comment.
does the issue specify changing the polling behaviour?
There was a problem hiding this comment.
- On SSE connection failure, fall back to polling if polling is enabled; otherwise continue SSE reconnection attempts
This reads to me like it is not enabled by default. Did I misread that @toddbaert ?
There was a problem hiding this comment.
my mistake, it is part of this new adr that hasn't been merged yet open-feature/protocol#69
2 participants
This PR
Disabled the default polling interval to make way for SSE.
Related Issues
Part of #1497
Notes
Follow-up Tasks
How to test