-
Notifications
You must be signed in to change notification settings - Fork 47
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: make updateToggles and sendMetrics available as public methods #226
Conversation
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.
Docs LGTM ✅
@@ -2268,3 +2289,54 @@ describe('Experimental options togglesStorageTTL enabled', () => { | |||
}); | |||
}); | |||
}); | |||
|
|||
describe('updateToggles', () => { |
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.
I wanted to suggest that we don't add the word toggles
in the public API since we already migrated most UI facing code to flags
instead of toggles
. But since we already have a public method called getAllToggles
in this SDK then introducing another convention adds more confusion rather than improving clarity.
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.
So no action needed IMO
README.md
Outdated
|
||
## Manage your own refresh mechanism | ||
|
||
If you do not want to use the internal refresh mechanism provided by the `refreshInterval` or `metricsInterval` options, you can handle it yourself using the public `updateToggles` and `sendMetrics` methods. |
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.
I'd suggest that we give more guidance to others willing to try this approach. What I'm missing is the information that we should skip calling the start
method on initialization.
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.
e.g. "You can opt out of the Unleash feature flag auto-refresh mechanism by not calling the start
method. In this case, it becomes your responsibility to call updateToggles
and sendMetrics
. This approach is useful in environments that do not support the setInterval
API, such as service workers."
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 need to call the start
method (no initial fetch otherwise) but what is needed is to set disableMetrics: true
and/or disableRefresh: true
in the options in order to handle it ourselves.
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.
Ah of course. You're right. Let's document it then to be extra clear in this section :)
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.
Doc updated
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.
I like the change as it opens more DIY options. Also very good test coverage. Thanks @jeremiewtd !
@jeremiewtd We've just released 3.6.0-beta.1 to npm. Feel free to use it in your context and provide feedback. Once we're happy with the results we can cut a non-beta 3.6.0 release. |
@kwasniew we have been using beta.0 for over two weeks and beta.1 for few days and everything is working as expected for us |
@jeremiewtd Thank you! We've been just discussing if we can cut a new version so your feedback is very timely :) cc @ivarconr |
thumbsup @kwasniew |
About the changes
In order to manage contexts where the internal refresh mechanism does not correspond to technical needs or constraints (Service Workers for example), we propose to make the
updateToggles
andsendMetrics
methods available as public as discussed in this conversation.Closes second part of #201