-
Notifications
You must be signed in to change notification settings - Fork 28
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
Move SlotBodyEnd
, StickyBottomBanner
& ReaderRevenueLinks
to Islands
#4126
Conversation
Size Change: -204 kB (-12%) 👏 Total Size: 1.51 MB
ℹ️ View Unchanged
|
…ReaderRevenueLinksIsland
Hi @OllysCoding @mxdvl this is on my TODO list to review, looks really interesting! Just wanted to point out that because this affects |
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.
Re @tjmw 's comment, shall we rename this PR to Move Slots to Islands
?
Such great work! I made an attempt to do this and failed but I really like the gentle approach you've taken here - this makes a lot of sense to me.
One thing, and I know I keep harping on about this but I would like to keep some of these more advanced Typescript features out of DCR. I do fully appreciate that there are some things that we could do that do add value but it's also true that, in isolation, they all add value. The problem is the costs of each of them compound and compound until the DCR codebase becomes intimidating and inaccessible which is very much not what we want for our core platform. This is code that is designed to be used for years to come by a wide range of developers with different skills and experience so we should be aiming for the lowest common denominator rather than choosing to write code that requires specific knowledge.
idApiUrl, | ||
stage, | ||
pageId, | ||
keywordsId, | ||
}: Props) => { | ||
}: Omit<Props, 'isDev' | 'switches' | 'isSensitive'>) => { |
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.
Can we omit Omit
please? I don't know what it does and would prefer to not spend my time reading the Typescript docs and would rather focus on reviewing this code. This cost is multiplied over time, every time a developer reaches this code we are now either expecting them to know this feature or we are expecting them to learn it which raises the bar to entry on DCR which is not what we want to do
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 did try to avoid it, but you then need two Props
types which are nearly identical. Either that or you have to use PropsA & PropsB
, which I wouldn't say is any clearer.
Any idea on how we can tackle this?
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.
To be honest, I can't suggest an alternative because I (genuinely) don't understand the code or what problem it is trying to solve. My guess is though that if DCR has been able to manage this long without this feature there probably is a way to make it 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.
If it helps, Omit<Object, Keys>
means this Object
without these Keys
. So in this case it's Props
minus the keys isDev
, switches
and isSensitive
.
In my experience it doesn't come up that often, but there may be certain use cases where it's convenient (like perhaps this one?) 🤷.
Docs are here: https://www.typescriptlang.org/docs/handbook/utility-types.html#omittype-keys
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.
@mxdvl on intersection types, we could add clarity and maybe have something to share elsewhere.
e.g.
// AB.ts
export type Props = {
switches: Switches;
isSensitive: boolean;
isDev: boolean;
}
// SlotBodyEnd.tsx
import { ABProps } from './AB.ts';
type Props = {
contentType: string;
// ...
}
const SlotBodyEndWithAB = (props: Props & ABProps) = {}
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.
@oliverlloyd in this case I think we’re coming up against some of the limitations of the Island pattern. The reason DCR managed this long without this feature is because we could confidently use composition. We found ourselves more comfortable with a single child to an Island, opting for a pattern where SlotBodyEnd
contains its own ABProvider
:
// Layout.tsx
<Island clientOnly={true}>
<SlotBodyEnd
propA="A"
propB="B"
propC="C"
propD="D"
/* … */
propM="M"
propN="N"
/>
</Island>
// SlotBodyEnd.tsx
export const SlotBodyEnd = ({ /* … */ }: Props) => {
return (
<WithABProvider
propA={A}
propF={F}
propN={N}
>
<SlotBodyEndWithAB
propA="A"
propB="B"
propC="C"
propD="D"
/* … */
propM="M"
/>
</WithABProvider>
);
};
The alternative to this would be to allow ABProvider to be importable, but that seems odd.
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.
Is the problem here that you don't want to have two sets of props for two different components? And want to connect the different types together and create one from the other? Asking because I am still a bit on the edge of understanding the goal with Omit
.
If my assumption here is right then I can see the benefit. You don't have to create a new type and have instead got a sort of abstraction that creates it for you. Another benefit is you're coupling the types together so any changes to the root type will be carried down into the child version of it.
The counter to that is we're adding complexity which we could avoid by having duplication and that the benefit of coupling is also a constraint.
limitations of the Island pattern
I don't think this is entirely true. The reason we're using composition is not really related to the islands pattern but is instead caused by the constraint that hooks cannot be called conditionally. The islands pattern really only means we need to add an ABProvider
, composition is how we would need to add that no matter if we were using an island or not.
In general though, I am happy to adopt these sorts of features if there were team consensus around them. You can have these sorts of discussions endlessly and there's really no right or wrong here.
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.
If my assumption here is right then I can see the benefit. You don't have to create a new type and have instead got a sort of abstraction that creates it for you. Another benefit is you're coupling the types together so any changes to the root type will be carried down into the child version of it.
The counter to that is we're adding complexity which we could avoid by having duplication and that the benefit of coupling is also a constraint.
By placing the ABProvider inside this component, we’re fully coupled, so the types might as well reflect it. There might be something I’m not seeing here. I’ll have a go at making WithABProvider
an importable.
I think, however, that @jamesgorrie’s suggestion is much better, and will have a go at implementing that instead!
EDIT: The new version does away with Omit
, and as a result is also more verbose 64e989f2e.
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 think some of the complexity lies in how we have decided Islands should be implemented if the need access to the useAB
hook.
For components to work, with AB hooks, in islands, we need to re-include the WithABProvider
. We have chosen to make this the responsibility of the component rather than the island (can't find the documentation for this).
This means we have the pattern:
BootReact
├─ WithABProvider
│ ├─ Island
│ │ ├─ ComponentWithABProvider
│ │ │ ├─ WithABProvider
│ │ │ │ ├─ Component
That means ComponentWithABProvider
require both the types for WithABProvider
and the Component
.
To this end, ComponentWithABProvider
would have the type of WithABProvider & ComponentProps
. This to me is quite easy to understand, or by using the Omit pattern, although for clarity I might use Omit<Props, ABProps>
.
The parent component would always have to implement the <Island><ComponentWithABProvider>
if the component uses the useAB
hook.
Maybe there is something we can do to simplify this?
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’m going to have a look at implementing the following approach:
Island
├─ WithABProvider
│ ├─ Component
There’s more discussion about this here: #3955
* | ||
* @deprecated prefer refactoring the code to use SWR natively | ||
*/ | ||
export const useSWRPromise = <Data extends unknown, Key extends string>( |
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 only use this code in two places so can we just duplicate it and not have the abstraction? If this code is already deprecated and we want to discourage it's use then we would achieve that much better by simply not creating it?
Also, inlining things is going to make it easier to read the code without the need to jump around files, which is always nice
And, if we don't need to pass things around so much we will need fewer types, it's all good stuff.
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.
Yes, the ideal would be to not create it… however this would mean rewriting the slots’s braze logic completely, which we felt would be best achieved in a subsequent PR.
When pairing on it, using generics helped us confirm that the compiler was in agreement with what we were trying to achieve. We definitely did not want to inline this code because while we strongly believe it needs to go, having two copies of a code that does something we don’t want is worse than a single copy.
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.
If we know we don't want people to use this abstraction then not creating it is probably the best way to stop that happening. Creating an abstraction that we don't want used is metaphorically asking for trouble.
Alternatively, you could rename it to a name that is less inviting? useSWRPromise_DEPRECATED
? Although I feel it is weird to create something which is immediately deprecated
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.
Would it be possible here to use a plain TS function which memoizes the returned Promise
? I think we have that pattern elsewhere in DCR. (I guess you'd need to combine with useEffect
/useState
to ensure this happens at the right time. This also might be relevant to Max's comment below, if we want to ensure buildBrazeMessages
is always called:
When it comes to making sure something is always called on a page, the current approach is having a script in the browser folder, like we do for bootCmp #3661, initDiscussion #3873 and coreVitals
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 actually prefer this being broken out in and purposely deprecated, and I'd support oliver's idea of naming it useSWRPromise_DEPRECATED
Why?
copy-pasting code is not a rare thing in DCR, so many people work on this codebase it's commonplace to go look for patterns in other places and use them for your own component/function. This is fine, most of this code will be abstracted when and if it's needed, so it's no big deal to go looking for an example of what you're trying to do.
The advantage of abstracting this pattern is that we can mark it globally as something not to use, so if in a years time this pattern still exists for some reason, but we're all gone, if somebody were to copy and paste the way we were using SWR to generate a promise, there's a decent chance nobody would even realise it was a deprecated pattern.
Having a function, clearly named useSWRPromise_DEPRECATED
would make it explicitly clear for any future developers that this is not a pattern we want to support. There's no chance it accidentally ends up in another place, at least not without the developers explicitly understanding why it was their only option.
Therefor I feel like it's the safer way to introduce this code, without risking it evolving or being assumed 'okay' in the future.
}: Props) => { | ||
}: Omit<Props, 'isDev' | 'switches' | 'isSensitive'>) => { | ||
const brazeMessages = useSWRPromise('braze-messages', () => | ||
buildBrazeMessages(idApiUrl), |
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.
To make sure I understand correctly - does the use of useSWRPromise
here effectively memoize the async work which buildBrazeMessages
does? Under the hood an instance of BrazeMessages
is returned which wraps the Braze SDK and I think we only ever want to have one of these on the page at a time. This used to happen because the instance was created in App
and was passed down to the relevant "slots" as a prop.
I think a difference I can see with the new version of this code is that we'll only call buildBrazeMessages
if one of (or both of) SlotBodyEnd
and StickyBottomBanner
exist in the page hierarchy? That's slightly different from before in App
where we would always call buildBrazeMessages
. The reason that distinction is important is because even if the slots/islands don't exist on the page, we still want to call buildBrazeMessage
currently as it does work to clear up after the SDK if the user has logged out or removed permissions for Braze to load. Any thoughts on how we could maintain that with this new approach? It's maybe not super relevant at the moment because StickBottomBanner
probably exists in every page layout? Even so it'd be nice to settle on an approach which works if that ever isn't true (now or in future).
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.
Yes, useSWRPromise
memoize the async work of buildBrazeMessages
. We might be able to use useSWRImmutable
under the hood to make sure there’s only ever one instance of the BrazeMEssages.
When it comes to making sure something is always called on a page, the current approach is having a script in the browser folder, like we do for other scripts that run on every page:
bootCmp
bootCmp
#3661initDiscussion
initDiscussion
(to fix permalinks) #3873coreVitals
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.
Worth noting that there's no standard way for code inside the Island to talk to code running through the script tags. If the fact that we have a Slot island on every page is not sufficient and we go the script way, implementing a mediator is a requirement.
One proposal for such a mediator is @ashishpuliyel's guardian/commercial#486.
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 reason that distinction is important is because even if the slots/islands don't exist on the page, we still want to call
buildBrazeMessage
currently as it does work to clear up after the SDK if the user has logged out or removed permissions for Braze to load. Any thoughts on how we could maintain that with this new approach?
This is a really interesting and useful point!
So the thing that SWR brings to the party for us is request deduplication. It takes any promised based function along with a key
value and wraps the call to it in some dedupe logic such that any subsequent call will return the same result.
The fact that SWR takes any function, not just a fetch call, is powerful. It means we can kind of dedupe anything. This is potentially very powerful for our islands architecture. In the situation we have static global state shared between islands, if we wrap the call to get this state using SWR then we effectively share it. Now, I used the words 'static' and 'effectively' there because the limitation to this SWR trick is we don't share dynamic changes to the state. If one island mutates it's state value the other islands are not aware of this.
To solve the issue you raised where we ideally always want to call this function, not just when a slot appears on the page, we could have a third island, one that only makes the call to buildBrazeMessages
. This will ensure that the call is always made but because of SWR deduplication it will not mean we make any additional actual executions of this function.
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.
To solve the issue you raised where we ideally always want to call this function, not just when a slot appears on the page, we could have a third island, one that only makes the call to
buildBrazeMessages
Ah cool, yeah that makes sense. If that fits with the way you (as in dotcom) are thinking about islands then I'd be in favour of adding that as part of this PR. That way even if it's true now that StickBottomBanner
appears on ever page, it guards against that changing in future and going un-noticed. I wonder what a good name is? Would this be the first island where there's no visual/UI element to 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.
Would this be the first island where there's no visual/UI element to it?
Not the first, no. Liveness sometimes returns a component but is mostly just a vehicle for code and CommercialMetrics never returns any html. There might be others.
18deaef
to
5e2d6c2
Compare
5e2d6c2
to
fb0c1a6
Compare
fb0c1a6
to
8d39aa6
Compare
rejecter = reject; | ||
}); | ||
|
||
useSWRImmutable(key, fetcher, { |
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. I really like how this communicates intent!
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.
Pending Tom's point about needed to ensure we always clear us (eg. we should always run the buildBrazeMessages
function) this look's great!
I appreciate there's a lot going on here though and I know you've got some other sub PRs but I'm happy for you to decide what's still needed and when best to merge things.
8d4e295
to
fa442b5
Compare
Co-authored-by: Olly <9575458+OllysCoding@users.noreply.github.com>
ReaderRevenueLinks
, SlotBodyEnd
& ReaderRevenueLinks
to Islands
dotcom-rendering/src/web/components/ReaderRevenueLinks.importable.tsx
Outdated
Show resolved
Hide resolved
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'm approving parts of this work but pending eyes from others
dotcom-rendering/src/web/layouts/InteractiveImmersiveLayout.tsx
Outdated
Show resolved
Hide resolved
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.
Overall this looks good to me! Think there are a few small improvements that could be made around islands idle/when visible/etc but otherwise I think this looks good!
Co-authored-by: Oliver Lloyd <oliverlloyd@users.noreply.github.com>
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.
Looks A Ok to me!
…/dotcom-rendering into mxdvl/braze-swr-islands
ReaderRevenueLinks
, SlotBodyEnd
& ReaderRevenueLinks
to IslandsReaderRevenueLinks
, SlotBodyEnd
, StickyBottomBanner
& ReaderRevenueLinks
to Islands
ReaderRevenueLinks
, SlotBodyEnd
, StickyBottomBanner
& ReaderRevenueLinks
to IslandsSlotBodyEnd
, StickyBottomBanner
& ReaderRevenueLinks
to Islands
</Portal> | ||
</React.StrictMode> | ||
); | ||
return null; |
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.
🎉
@jamesgorrie @mxdvl @tjmw This PR looks like it's ready to merge! I think this part of the PR is especially nice We've all contributed to this work recently and @AshCorr , @OllysCoding and I have been working today to finalise the changes. We're planning on making a final deployment to |
Hi @oliverlloyd thanks for the heads up! Yep would love to take a final glance on code before this gets merged. Would you mind pinging me when it's there? |
I've deployed this branch to |
What does this change?
Migrates
StickyBottomBanner
,SlotBodyEnd
&ReaderRevenueLinks
into their own respective islands. These three components need moved together as they had shared dependencies that could not run in both Islands and App.tsx.To support the move to the Islands pattern, we introduce
useBraze
, a hook which makes use ofuseSWRImmutable
to ensure we initialise braze only once per page.This PR also introduces the
BrazeMessaging
Island, a canonical usage ofuseBraze
to ensure that braze is properly initialised on every page.Small improvements
Why?
Islands are the new way: #3629 🏝️