Skip to content

Conversation

mxdvl
Copy link
Contributor

@mxdvl mxdvl commented Oct 3, 2023

depends on #8967...

What does this change?

  • adds a priority prop to Island, which corresponds to the priorities of the scheduler
  • prioritises each instance of Island via the new prop

Important

This PR does not affect way in which Islands are loaded/hydrated right now.

A subsequent PR will use the new priority prop to orchestrate hydration with the scheduler.

Why?

Will enable us to ensure high priority islands are run on the page before other, lower priority tasks.

Reviewing

A lot of files are touched, but the main changes are in dotcom-rendering/src/components/Island.tsx (and its test file).

All other changes should just be adding the priority prop to existing Islands.#9013

@mxdvl mxdvl changed the title feat: add priority to Island Add priorities to Island Oct 3, 2023
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

Size Change: 0 B 🆕

Total Size: 0 B

compressed-size-action

@mxdvl mxdvl force-pushed the oj/islands/defer-props branch from abef462 to 1e2b436 Compare October 4, 2023 09:37
@sndrs sndrs force-pushed the oj/islands/defer-props branch from 1e2b436 to 1d8e3ee Compare October 4, 2023 10:47
@mxdvl mxdvl force-pushed the oj/adaptive/island-priorities branch from eaa286c to 677d42c Compare October 4, 2023 11:00
@sndrs sndrs force-pushed the oj/adaptive/island-priorities branch 7 times, most recently from b285560 to 5e40ec2 Compare October 4, 2023 11:53
@mxdvl mxdvl force-pushed the oj/adaptive/island-priorities branch from 5e40ec2 to d284108 Compare October 4, 2023 12:17
@sndrs sndrs changed the title Add priorities to Island Adaptive site: add priorities to Island Oct 4, 2023
@sndrs sndrs marked this pull request as ready for review October 4, 2023 13:18
@sndrs sndrs requested a review from a team as a code owner October 4, 2023 13:18
mxdvl and others added 2 commits October 4, 2023 14:34
Non-critical islands _must_ be deferred!

Co-authored-by: Alex Sanders <alex@sndrs.dev>
@sndrs sndrs force-pushed the oj/adaptive/island-priorities branch from d284108 to dadb0eb Compare October 4, 2023 13:34
@sndrs sndrs changed the title Adaptive site: add priorities to Island Adaptive site: add priority prop to Island Oct 4, 2023
@mxdvl mxdvl added the run_chromatic Runs chromatic when label is applied label Oct 4, 2023
Copy link
Contributor

@cemms1 cemms1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ 💃 ✨

Base automatically changed from oj/islands/defer-props to main October 5, 2023 12:57
@mxdvl mxdvl merged commit 71b6ce3 into main Oct 5, 2023
@mxdvl mxdvl deleted the oj/adaptive/island-priorities branch October 5, 2023 13:41
@@ -400,7 +400,7 @@ export const renderElement = ({
return (
// Deferring interactives until CPU idle achieves the lowest Cumulative Layout Shift (CLS)
// For more information on the experiment we ran see: https://github.com/guardian/dotcom-rendering/pull/4942
<Island defer={{ until: 'idle' }}>
<Island priority="critical" defer={{ until: 'visible' }}>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We made this change because you cannot defer until idle for a critical priority – we should probably have brought more attention to this change to @guardian/commercial-dev

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a catchup with @mxdvl regarding this change it turns out it is safe to use idle in priority critical because there is a timeout in whenIdle function. There is a new change introduced here #11077

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants