-
Notifications
You must be signed in to change notification settings - Fork 30
Client for reading Fastly dictionary ab test state #14344
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Imogen Hardy <[email protected]>
@@ -51,6 +51,15 @@ void (async () => { | |||
}, | |||
); | |||
|
|||
void startup( |
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.
The method on the window needs to be available ASAP so that commercial and other client side code is able to use it.
Alternatively we could move this to libs and import it?
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.
Having it in the lib and not relying on DCR to have added it the window would remove the risk of a race condition where commercial tries to call something on the window that hasn't been defined? That seems a plus for moving it to a lib. Are there any negatives of moving this to a lib?
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.
It's quite a small amount of code, the duplication's effect on bundle sizes would be negligible I think.
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.
I would suggest adding a comment here initially, explaining what's going on and then convert to a lib later on if required?
098ff9a
to
c77cfc8
Compare
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
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.
Pull Request Overview
This PR introduces a new beta A/B testing client framework for reading Fastly dictionary A/B test state, designed to replace the current @guardian/ab-core
framework. The implementation supports both server-side and client-side A/B testing with separate data sources and provides a React hook for DCR usage.
- Adds
BetaABTests
class for managing A/B test participation with server-side and client-side support - Implements
useBetaAB
React hook for accessing A/B test state in components - Integrates client-side A/B test reading from cookies and server-side tests from configuration
Reviewed Changes
Copilot reviewed 30 out of 61 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
src/experiments/lib/beta-ab-tests.ts |
Core beta A/B testing class implementation with server/client-side logic |
src/client/abTesting.ts |
Client-side A/B test initialization and cookie parsing logic |
src/lib/useAB.ts |
React hooks for beta A/B testing integration |
src/components/SetABTests.importable.tsx |
Integration of beta A/B tests into existing setup flow |
src/types/config.ts |
Type definition updates for new serverSideABTests property |
Multiple schema/render files | Addition of serverSideABTests configuration throughout the application |
export const useBetaAB = (): BetaABTestAPI | undefined => { | ||
const { data } = useSWRImmutable( | ||
'beta-ab-tests', | ||
() => new Promise<BetaABTestAPI>(() => {}), |
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 promise never resolves, which means the hook will always return undefined. The promise should either resolve with a real implementation or use a different approach to initialize the data.
() => new Promise<BetaABTestAPI>(() => {}), | |
() => undefined, |
Copilot uses AI. Check for mistakes.
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.
We want it to never resolve as it will be replaced.
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.
How does this work?
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 was copied from the current client useAB
added in #4200, it is initially set to a promise that won't resolve, when SetABTests.importable
runs it sets it to the initialised ab test API.
It needs to be a promise, not sure if it necessarily needs to be a promise that doesn't resolve. Promise.resolve(undefined)
might do the trick too.
/** | ||
* The 'abTests' is for use by external scripts that need to | ||
* access A/B test information. Do not use this directly | ||
* in DCR, instead use the `useAB` hook as it is csr/ssr aware. | ||
*/ |
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.
nice 👏
export const useBetaAB = (): BetaABTestAPI | undefined => { | ||
const { data } = useSWRImmutable( | ||
'beta-ab-tests', | ||
() => new Promise<BetaABTestAPI>(() => {}), |
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.
How does this work?
@@ -51,6 +51,15 @@ void (async () => { | |||
}, | |||
); | |||
|
|||
void startup( |
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.
I would suggest adding a comment here initially, explaining what's going on and then convert to a lib later on if required?
Yup good idea 👍 |
Co-authored-by: Imogen Hardy [email protected]
What does this change?
This is based off @i-hardy's work that was integrated into our proof of concept branch for the new framework.
Adds a "beta" client for detecting if a user is in a test and/or variant, so that behaviour/rendering can be changed if a user is in said test/variant.
Client side tests are read from the
gu_client_ab_tests
cookie, and server side tests read fromserverSideABTests
on the data from frontend/window.There's a react hook for use in DCR, as well as a method on the window for other bundles such as commercial to use. The hook works both client and server side, when running client side it will return both server and client side tests, but when used server side will only return server side tests.
Why?
As all test participation is calculated at the edge, the current frameworks use of
@guardian/ab-core
is no longer needed, with the Ophan reporting integrated here in DCR.We've created a new "beta" client so that we can compare the old and new frameworks behaviour.