-
-
Notifications
You must be signed in to change notification settings - Fork 658
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: Persist dependent features #4772
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
enabled boolean DEFAULT true NOT NULL, | ||
variants JSONB DEFAULT '[]'::jsonb NOT NULL, | ||
PRIMARY KEY (parent, child), | ||
FOREIGN KEY (parent) REFERENCES features (name) ON DELETE RESTRICT, |
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.
should not be able to delete parent flag when child flags still exist
variants JSONB DEFAULT '[]'::jsonb NOT NULL, | ||
PRIMARY KEY (parent, child), | ||
FOREIGN KEY (parent) REFERENCES features (name) ON DELETE RESTRICT, | ||
FOREIGN KEY (child) REFERENCES features (name) ON DELETE CASCADE |
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.
deleting a child flag should remove the dependency
enabled: featureDependency.enabled, | ||
}; | ||
if ('variants' in featureDependency) { | ||
serializableFeatureDependency.variants = JSON.stringify( |
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.
arrays need to be stringified
} | ||
await this.db('dependent_features') | ||
.insert(serializableFeatureDependency) | ||
.onConflict(['parent', 'child']) |
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.
this gives us upsert semantics
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.
LGTM. I think it makes sense to use foreign keys for this.
About the changes
Persisting dependent features toggles.
parent name, child name, whether parent is enabled or disabled and parent variants that we want to match on
Important files
Discussion points
Database Table Design Decisions:
Both 'parent' and 'child' are set up as foreign key relationships. This allows us to easily display these relationships in the admin user interface and prevents invalid bindings.
While the 'variants' could technically use a foreign key relationship to enforce valid values, it might be overcomplicated. Given that both strategy variants and feature variants are also stored as JSONB, introducing such constraints could be redundant. Moreover, establishing foreign keys to multiple tables presents its challenges. Using JSONB for 'variants' also provides flexibility for potential operator support in the future.
I've decided to address the comprehensive feature-with-dependencies read model/view in a separate PR. This is because introducing it would be a more intricate and potentially riskier change compared to merely saving dependencies.