bootCmp - #3661
bootCmp#3661
bootCmp#3661Conversation
…nent.tsx Co-authored-by: Max Duval <max.duval@theguardian.com>
|
Size Change: -405 B (0%) Total Size: 3.13 MB
ℹ️ View Unchanged
|
|
As CMP is comparatively slow, I would be tempted to keep it very high up the priority list. We need it to complete in order to do anything that requires consent, such as:
|
Noted 👍 |
|
Just making a note here that @OllysCoding spoke to me about the bundle size increase of 4Kb we're seeing here and suggested we look at dependency caching with webpack. We're going to pair on this tomorrow |
…re to have it in scope
| undefined, | ||
| ); | ||
|
|
||
| useOnce(() => { |
There was a problem hiding this comment.
This is just a hunch... although this will initiate CMP and set consent state on a second pass it won't block the render of YoutubeAtom on the first pass without consent.
YoutubeAtom will render its first and second pass (after its internal useEffect) without consent and therefore default to disabled ads.
Previously the HydrateOnce for YoutubeBlockComponent would waitFor={[consentState]}
I think...
There was a problem hiding this comment.
Thanks @arelra. I think you're right. Previously we waited for consentState in App.tsx using a waitFor but we're not doing that here. I think I saw it suggested that we could fix this by making consentState a dependency for useOnce to run, which I think it true. This would essentially replicate the logic we had previously. Ahead of that though, I'm investigating if there are some optimisations that can be made in the YouTubeAtom itself. I'll update back here when I've done this.
There was a problem hiding this comment.
You might be already considering this but I think completely separating the overlay from the player will help reduce complexity of YoutubeAtom quite a bit and allow blocking of only whats required - i.e. instantiation of the player.
It will just need a method to trigger the play from the overlay to the player.
There was a problem hiding this comment.
Based on this discussion, I looked deeper at how we render You Tube videos and discovered that we can improve how we do this.
@mchv pointed out a great lib to me earlier which defers the loading of the you tube javascript until the user clicks the poster overlay. My first response was to say that we already do this but when I looked more I realised we don't! So I've added this feature to our YouTubeAtom
Whilst doing this I also added a defer to wait for consentState which, if we're happy with that approach` would address the issues in this thread, without the need to manage waiting for state locally.
I'm also happy to put the waitFor in and not have YouTubeAtom manage this (we could make it required) or some combination of the both. Whichever api makes the best sense to the group is fine by me.
There was a problem hiding this comment.
Is useOnce required if the dependency array is [] ?
If not you could just have a useEffect with [] ?
|
The T&C docs will need updating to point at this new location I think: https://github.com/guardian/transparency-consent-docs/blob/main/docs/in-the-browser.md |
Great point! Thanks for raising this |
| undefined, | ||
| ); | ||
|
|
||
| useOnce(() => { |
There was a problem hiding this comment.
Is useOnce required if the dependency array is [] ?
If not you could just have a useEffect with [] ?
What does this change?
This PR moves the initialisation of
cmpout ofApp.tsxand into it's own boot script.Along with moving the initialisation, I also moved down some code from
App.tsxthat got theconsentStateand passed it down to theYouTubeBlockComponent. In doing this, I made it a dynamic import.Why?
This is part of a wider refactor to remove logic from
App.tsxThings still under consideration
YouTubeBlockComponentbootCmpscript later, as a lower priority script tag?