feat: useAB with SWR (no Context or Provider) #4200
Merged
Conversation
This file contains 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
mxdvl
requested review from
a team,
JamieB-gu,
marsavar and
iainjchambers-guardian
as code owners
|
Size Change: -26.6 kB (-2%) Total Size: 1.53 MB
ℹ️ View Unchanged
|
mxdvl
force-pushed
the
mxdvl/islands/ab-swr
branch
from
53e23d1 to
a6ffb21
Compare
mxdvl
force-pushed
the
mxdvl/islands/ab-swr
branch
from
a6ffb21 to
9b94183
Compare
|
This might not be fully baked yet… @OllysCoding feel free to pick up and modify as necessary. I think the failing tests are because App.tsx creates a distinct instance of |
mxdvl
force-pushed
the
mxdvl/islands/ab-swr
branch
from
9b94183 to
566e892
Compare
mxdvl
commented
Mar 8, 2022
mxdvl
force-pushed
the
mxdvl/islands/ab-swr
branch
from
566e892 to
ea52d8d
Compare
mxdvl
force-pushed
the
mxdvl/islands/ab-swr
branch
from
ea52d8d to
3ac7f87
Compare
mxdvl
force-pushed
the
mxdvl/islands/ab-swr
branch
from
3ac7f87 to
4a72524
Compare
mxdvl
force-pushed
the
mxdvl/islands/ab-swr
branch
from
4a72524 to
6eac789
Compare
oliverlloyd
approved these changes
Mar 25, 2022
dotcom-rendering/src/web/components/ReaderRevenueLinks.stories.tsx
Outdated
Show resolved
Hide resolved
| @@ -77,6 +71,13 @@ export const Page = ({ CAPI, NAV, format }: Props) => { | |||
| shouldHideReaderRevenue={CAPI.shouldHideReaderRevenue} | |||
| /> | |||
| </Island> | |||
| <Island clientOnly={true}> | |||
There was a problem hiding this comment.
Not really sure if this is a problem or not but is there a risk that we have code in other islands that starts to execute prior or in parallel to this initialisation?
We currently deal with dependency by putting things in the head but perhaps we can just early return in local components if there's a dependency?
There was a problem hiding this comment.
I think this is down to consumer to make this decision. There’s a chance that the ABTestApi is undefined. Most cases are already using a useOnce, so it looks safe.
mxdvl
changed the title
A hook which returns the AB Test Api when available, or undefined otherwise. Leverages an immutable SWR to satisfy all requests to the AB Core. As soon as the tests are available, all instances of the useAB hook will render. Previously, we had an AB Provider wrapped around App.tsx. With Islands, each island need to provide its own Provider, leading to much prop repetition. Co-authored-by: Olly <OllysCoding@users.noreply.github.com>
Favour lib/useAB instead – #4200 Fix tests for new useAB hook Co-authored-by: Olly <9575458+OllysCoding@users.noreply.github.com>
By using AB & setABTests in preview.js
mxdvl
force-pushed
the
mxdvl/islands/ab-swr
branch
from
13b0a37 to
2307f58
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
Leverages SWR to provide the
ABTestAPIviauseAB. Instead of using aProvider, we have an Island dedicated to mutating the SWR cache (SetABTests).This introduces a new pattern: SWR’s
mutate, which is an a tool usually built for optimistic data mutation, where we don’t wait for the promise to resolve. In this specific case, the promise itself will never resolve, and we have to set the value ourselves.Why?
We currently need to have AB Providers for each Island that relies on the AB Test API. This leads to duplication of props and a lot more boilerplate code.
We want the platform to be as easy as possible to use.
See #3955