WPB-21964: move feature flags logic to wire-subsystems #4941
Conversation
zebot
added
the
ok-to-test
battermann
left a comment
battermann
left a comment
There was a problem hiding this comment.
please add a changelog
There was a problem hiding this comment.
does this have to stay in galley? why?
There was a problem hiding this comment.
It depends on things only definable in galley, such as Opts.
There was a problem hiding this comment.
That alone is solvable by extracting a dedicated config for the subsystem. Are there still other reasons?
There was a problem hiding this comment.
It seems so, but it is the wrong tradeoff.
Pros:
- interpreters and effects are co-located (for one usage)
Cons:
- high coupling of
galleywith all packages depending onwire-subsystems - low cohesion, as all changes will impact all the dependant packages
Having separated effects definitions/interpreters, enforcing high cohesion and low coupling, is the purpose of effects systems.
| global <- inputs (view $ settings . checkGroupInfo) | ||
| guard (global == Just True) | ||
| mls <- getFeatureForTeam @MLSConfig tid | ||
| mls <- getFeatureForTeam @_ @MLSConfig tid |
There was a problem hiding this comment.
Can we change the order of the type parameters so that we do not have to pass a placeholder type?
There was a problem hiding this comment.
I'm not sure, it's polysemy defined
There was a problem hiding this comment.
Just tried with foralls at effect level, it does not work
|
@akshaymankar @battermann I have moved the interpreters in |
battermann
left a comment
battermann
left a comment
There was a problem hiding this comment.
FeatureConfigStore is not the right abstraction. It does CRUD operations via TeamFeatureStore + compute logic + membership checks. This looks more like a subsystem. I would suggest to rename it to FeatureConfigSubsystem.
Also FeatureConfigCompute does not have to be an effect. What do we gain? Do we ever want to mock it independently of the other store/orchestration layer? I would suggest to either inline this into the FeatureConfigSubsystem or have it in a plain module without all the Polysemy overhead.
libs/wire-subsystems/src/Wire/FeaturesConfigCompute/Interpreter.hs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This is doing more than just store operations. See overall PR comment.
There was a problem hiding this comment.
I know that this was just moved, but can we use code gen with makeSem instead of explicitly wrap each operation with send to reduce boilerplate?
There was a problem hiding this comment.
We cannot, there are singletons work in it.
… `FeaturesConfigSubsystem` (Leif feedabcks)
Checklist
changelog.d