-
Notifications
You must be signed in to change notification settings - Fork 290
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: evaluate list of self supported protocols on team protocols update #15888
feat: evaluate list of self supported protocols on team protocols update #15888
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #15888 +/- ##
==========================================
- Coverage 44.51% 44.50% -0.01%
==========================================
Files 673 674 +1
Lines 22735 22755 +20
Branches 5170 5175 +5
==========================================
+ Hits 10120 10127 +7
- Misses 11321 11334 +13
Partials 1294 1294 |
// When we receive a `feature-config.update` event, we will refetch the entire feature config | ||
await this.updateFeatureConfig(); | ||
const {prevFeatureList} = await this.updateFeatureConfig(); | ||
this.emit('featureUpdated', {event, prevFeatureList}); |
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.
suggestion: maybe we could just send the previous value rather than the entire feature config, WDYT?
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.
Well, I tried to do it but didn't manage to achieve it type-wise. When accessing prevFeatureList[event.name]
at this point, the type will not be properly narrowed down to e.g. FeatureMLS
(we don't know what's event.name
at compile time, it's FEATURE_KEY
type)
Event data is nicely tied to its name.
But we cannot tie specific feature to it by FEATURE_KEY at this point. It's still union of different features.
Maybe it's possible but i cannot find a way? Any ideas?
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 nevermind we can probably change that later, no biggies here 👌
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.
question: Will this work when the entire feature list is updated (that happens, for example, once every 24h)?
We might need to account for this as well
@atomrc EDIT: Nevermind, we don't have access to event there, I will just handle this case separately. |
Description
Every time team (or backend) admin changes the list of supported protocols (in feature config) we want to also re-evaluate the list of self user's supported protocols. This PR adds this functionality. Every time we receive feature config update, and see that
"mls"
was the feature that had been updated, we compare the local list of current supported protocols stored locally in team config with the list received in"mls"
feature config update. If supported protocols were changed, we re-evaluate the list of self supported protocols and update it on backend.Checklist