Conversation
mxdvl
requested review from
a team,
JamieB-gu,
iainjchambers-guardian and
marsavar
as code owners
March 4, 2022 17:31
|
Size Change: +518 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
oliverlloyd
reviewed
Mar 7, 2022
oliverlloyd
left a comment
Contributor
There was a problem hiding this comment.
Are we saying here that within switches there is actually a subset of switches used for ab tests and that sometimes in the code these are getting confused with the main switches used as feature flags? Does this perhaps surface a larger problem that we should really have two properties on the model?
If the above is true, then ideally this transformation would happen earlier in the stack, in which case this change could be made into an enhancer. We use enhancers for the types of model changes that we:
- want to happen now so that we can write the rendering code the way we think it should be be but
- also want to move up to a higher level in the stack
By using enhancers we make any transition up in future easier (because the mutation is contained) and we also make writing rendering code easier as the model fits the right pattern.
|
|
||
| export const filterABTestSwitches = (switches: Switches): ABTestSwitches => | ||
| Object.fromEntries( | ||
| Object.entries(switches).filter(([key]) => key.startsWith('ab')), |
Contributor
There was a problem hiding this comment.
Is there a convention that switches used for ab tests are given the ab prefix and this function is built on top of that pattern?
Contributor
Author
There was a problem hiding this comment.
Yes, this is a convention followed in frontend and the ab-testing framework expects it.
Sadly I couldn’t find documentation outside of the actual LoC where this stuff is evaluated. I tried to bring a change to the ab-testing library to use a template string to indicate this fact with ab${string}, but couldn’t get the repo to behave.
Contributor
Worth keeping in mind that we recently refactored If we wanted to use an enhance pattern for this solution, we'd need to re-introduce some kind of enhancement protocol which works for the wider CAPI object, while making sure it doesn't prevent support for our new endpoints like |
Contributor
|
We already have enhanceStandfirst which I think follows the pattern we would need here. Right now we have So maybe we could or something like that? |
Merged
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this change?
Add a method to filter out unused switches.
Why?
We should only pass required props to
Islandchildren.Before
After