Skip to content

Adaptive site: refactor Island defer props#8967

Merged
mxdvl merged 2 commits intomainfrom
oj/islands/defer-props
Oct 5, 2023
Merged

Adaptive site: refactor Island defer props#8967
mxdvl merged 2 commits intomainfrom
oj/islands/defer-props

Conversation

@mxdvl
Copy link
Copy Markdown
Contributor

@mxdvl mxdvl commented Oct 2, 2023 •
edited
Loading

What does this change?

Refactor the defer island prop.

Before

<Island />
<Island deferUntil="idle" />
<Island deferUntil="visible" />
<Island deferUntil="visible" rootMargin="100px" />
<Island deferUntil="interaction" />
<Island deferUntil="hash" />

After

<Island />
<Island defer={{until: "idle"}} />
<Island defer={{until: "visible"}} />
<Island defer={{until: "visible", rootMargin: "100px"}} />
<Island defer={{until: "interaction"}} />
<Island defer={{until: "hash"}} />

See Island.tsx specifically

Why?

This simplifies the types for Islands that are not deferred until visible. For example:

type IdleProps = {
-	deferUntil: 'idle';
+	until: 'idle';
-	rootMargin?: never;
};

type VisibleProps = {
-	deferUntil: 'visible';
+	until: 'visible';
	rootMargin?: string;
};

This simplification will greatly reduce the complexity of introducting a new priority prop (#8970), which, like VisibleProps, only allows specific combinations of props. This adheres to the principle of making impossible states unrepresentable. For example, valid combinations will be:

Critical Feature Enhancement
none ☑️
idle ☑️ ☑️
visible ☑️ ☑️ ☑️
hash ☑️ ☑️ ☑️
interaction ☑️ ☑️ ☑️

This change enables a future where rather than props looking something like this:

type NonDeferredProps = {
	deferUntil?: never;
	rootMargin?: never;
	priority: "critical";
};

type VisibleProps = {
	deferUntil: 'visible';
	rootMargin?: string;
	priority: "critical" | "feature" | "enhancement";
};

type InteractionProps = {
	deferUntil: 'interaction';
	rootMargin?: never;
	priority: "critical" | "feature" | "enhancement";
}

type HashProps = {
	deferUntil: 'hash';
	rootMargin?: never;
	priority: "critical" | "feature" | "enhancement";
}

type IdleProps = {
	deferUntil: 'idle';
	rootMargin?: never;
	priority: "feature" | "enhancement";
};

They can now be written something like this:

type VisibleProps = {
	until: 'visible';
	rootMargin?: string;
}

type InteractionProps = {
	until: 'interaction';
}

type HashProps = {
	until: 'hash';
}

type IdleProps = {
	until: 'idle';
}

type CriticalProps = {
	priority: 'critical';
	defer?: VisibleProps | InteractionProps | HashProps;
}

type FeatureProps = {
	priority: 'feature';
	defer: VisibleProps | InteractionProps | HashProps | IdleProps;
}

type EnhancementProps = {
	priority: 'enhancement';
	defer: VisibleProps | InteractionProps | HashProps | IdleProps;
}

Which means we no longer have to switch off invalid combinations with key?: never, we simply do not create them (they can be discriminated away). This greatly improves the ratio of signal (key: 'value') v noise (key?: never).

See working demo in a TS playground

Follow-up on #8948 & #8944

Screenshots

N/A

@mxdvl mxdvl added the Health label Oct 2, 2023
@mxdvl mxdvl requested a review from a team as a code owner October 2, 2023 17:09
@mxdvl mxdvl changed the title Refactor Ilsand defer props Refactor Island defer props Oct 2, 2023
@mxdvl mxdvl force-pushed the oj/islands/never-client-side-only branch from 80a6c46 to 3231a33 Compare October 2, 2023 17:17
@mxdvl mxdvl force-pushed the oj/islands/defer-props branch from 312ac01 to 30ce3c2 Compare October 2, 2023 17:19
@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 2, 2023 •
edited
Loading

Size Change: 0 B 🆕

Total Size: 0 B

compressed-size-action

Base automatically changed from oj/islands/never-client-side-only to main October 3, 2023 06:48
@mxdvl mxdvl force-pushed the oj/islands/defer-props branch from 30ce3c2 to abef462 Compare October 3, 2023 08:28
@mxdvl mxdvl added the run_chromatic Runs chromatic when label is applied label Oct 3, 2023
@mxdvl mxdvl force-pushed the oj/islands/defer-props branch from abef462 to 1e2b436 Compare October 4, 2023 09:37
@mxdvl @sndrs
refactor(Island): defer prop as union
1d8e3ee 
prevent impossible states from being representable

Co-authored-by: Alex Sanders <[email protected]>
@sndrs sndrs force-pushed the oj/islands/defer-props branch from 1e2b436 to 1d8e3ee Compare October 4, 2023 10:47
@sndrs sndrs changed the title Refactor Island defer props Adaptive site: refactor Island defer props Oct 4, 2023
@sndrs sndrs mentioned this pull request Oct 4, 2023
Merged
@cemms1 cemms1 modified the milestone: Health Oct 4, 2023
@cemms1 cemms1 removed the Health label Oct 4, 2023
cemms1
cemms1 approved these changes Oct 5, 2023
View reviewed changes
Copy link
Copy Markdown
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.

✨ 💃 ✨

cemms1
cemms1 reviewed Oct 5, 2023
View reviewed changes
dotcom-rendering/src/components/Island.test.tsx
Comment on lines +37 to +39
// this is just to stop Jest complaining about no tests
test('this is not a real test, ignore it, it tells you nothing useful 💃', () =>
undefined);
Copy link
Copy Markdown
Contributor

@cemms1 cemms1 Oct 5, 2023

Choose a reason for hiding this comment

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

a future improvement could be to devise a neat way to check types like this in the future, hopefully without the need for this test

Copy link
Copy Markdown
Member

@sndrs sndrs Oct 5, 2023

Choose a reason for hiding this comment

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

i promise to look at this properly 🤠

Copy link
Copy Markdown
Member

@sndrs sndrs Oct 5, 2023

Choose a reason for hiding this comment

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

just to join to the dots #9033

@mxdvl mxdvl mentioned this pull request Oct 5, 2023
Open
@mxdvl mxdvl merged commit 08ef588 into main Oct 5, 2023
@mxdvl mxdvl deleted the oj/islands/defer-props branch October 5, 2023 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sndrs sndrs sndrs left review comments

@cemms1 cemms1 cemms1 approved these changes

@abeddow91 abeddow91 abeddow91 approved these changes

Assignees

No one assigned

Labels

dotcom-rendering run_chromatic Runs chromatic when label is applied 📶 Open Journalism

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mxdvl @sndrs @abeddow91 @cemms1