Skip to content
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: dependent features in playground #4930

Merged
merged 5 commits into from
Oct 5, 2023

Conversation

kwasniew
Copy link
Contributor

@kwasniew kwasniew commented Oct 5, 2023

About the changes

Playground can resolve dependent feature toggles

Parent dependency not satisfied UI
Screenshot 2023-10-05 at 09 36 56

Parent dependency not satisfied and feature is disabled
Screenshot 2023-10-05 at 09 36 31

  • most of the code copied from the Node SDK with minor tweaks since the enabled/disabled resolution happens outside of the client in playground (in the service)
  • UI tests will be added in the next PR since I need to add new tests for playground UI

Important files

Discussion points

@vercel
Copy link

vercel bot commented Oct 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 5, 2023 10:07am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Oct 5, 2023 10:07am

return !feature.strategies?.data?.find((strategy) =>
DEFAULT_STRATEGIES.includes(strategy.name),
return (
feature.strategies?.data?.length > 0 &&
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was a bug when strategies were an empty list

@@ -19,6 +19,7 @@ export interface PlaygroundFeatureSchema {
strategies: PlaygroundFeatureSchemaStrategies;
/** Whether the feature is active and would be evaluated in the provided environment in a normal SDK context. */
isEnabledInCurrentEnvironment: boolean;
hasUnsatisfiedParent: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will be generated with orval with next PR, this one is added manually

@@ -57,13 +58,63 @@ export default class UnleashClient {
);
}

isParentDependencySatisfied(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copied from Node SDK with minor tweaks

if (!feature.isEnabledInCurrentEnvironment) {
return 'If environment was enabled';
}
return '';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not occur, will take care of it with the next PR with extensive test suite

@@ -19,6 +19,7 @@ export interface PlaygroundFeatureSchema {
strategies: PlaygroundFeatureSchemaStrategies;
/** Whether the feature is active and would be evaluated in the provided environment in a normal SDK context. */
isEnabledInCurrentEnvironment: boolean;
hasUnsatisfiedParent?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will be auto generated from orval, for now I added it manually

@@ -3,5 +3,5 @@ import { FeatureDependency, FeatureDependencyId } from './dependent-features';
export interface IDependentFeaturesStore {
upsert(featureDependency: FeatureDependency): Promise<void>;
delete(dependency: FeatureDependencyId): Promise<void>;
deleteAll(child: string): Promise<void>;
deleteAll(child?: string): Promise<void>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useful for test teardown

clientContext,
),
variant: isEnabled
? client.forceGetVariant(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this version of a client does not check feature enabled status this code was returning variants for disabled features. This PR fixes this bug

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't this by design? "If this feature is enabled, it would return X variant"?

clientContext,
),
variant: isEnabled
? client.forceGetVariant(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't this by design? "If this feature is enabled, it would return X variant"?

@kwasniew kwasniew merged commit 2c7587b into main Oct 5, 2023
13 of 15 checks passed
@kwasniew kwasniew deleted the dependent-features-in-playground branch October 5, 2023 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants