Added feature flag strategies #19
Conversation
|
Just been testing this and it looks like there is a problem with the userId hook. i will debug and commit an update |
There was a problem hiding this comment.
Thank for the merge request but there seems to be an issue with the approach. I would not put all the strategy context in the main configuration object, it assumes you already know the strategy variables at the moment of initialisation. In some cases (like used ID) it might not be the case. Instead it would be better to pass the context when calling the flag. This way it will stay more flexible for future strategies.
Also added some inline notes in the code.
Thanks again for the MR! 🙏
| Authorization: 'token123' | ||
| } | ||
| }, | ||
| userId: userId, |
There was a problem hiding this comment.
move this to a separate context object (explained in MR comment)
| * Get the user ID using the hook passed into the config | ||
| */ | ||
| public getUserId(): string | undefined { | ||
| if(this.config.userId) { return this.config.userId} |
There was a problem hiding this comment.
if statement can be removed. If this will be moved to the context the entire method can be removed as well
| children: ReactNode, | ||
| }; | ||
|
|
||
| const checkFlagStrategies = (flag: FlagValue, userId: string | undefined) => { |
There was a problem hiding this comment.
would be nice to split up each strategy in a separate file to keep it maintainable when more and more complex strategies will be added
|
|
||
| if (flag.enabled && flag.strategies) { | ||
| // check if strategies match | ||
| const flagStrategyResult = checkFlagStrategies(flag, flagsClient ? flagsClient.getUserId() : undefined); |
There was a problem hiding this comment.
No need for a new variable here.
isEnabled = checkFlagStrategies(flag, flagsClient ? flagsClient.getUserId() : undefined);
| isEnabled = flagStrategyResult; | ||
| } else { | ||
| // if the flag is disabled or no strategies are present then result is false | ||
| isEnabled = false; |
There was a problem hiding this comment.
When no strategy is present we should always fall back to the default strategy. It is the default.
| uri: process.env.REACT_APP_FLAGS_CTX_URI || DEFAULT_FEATURES_URI | ||
| uri: process.env.REACT_APP_FLAGS_CTX_URI || DEFAULT_FEATURES_URI, | ||
| // the userId to check strategies against | ||
| userId: process.env.REACT_APP_FLAGS_CTX_USER_ID || '' |
There was a problem hiding this comment.
Chances of knowing the user ID at this point are really small. As suggested, this should be handled in a context which is passed to the client at the moment we need to check a flag.
2 participants
I have added the implementation for the Gitlab supported strategies.
Tests and README have also been updated to reflect this.