Merge pull request #3839 from guardian/oliver/discussion-island

`Discussion` as an island
guardian · Jan 21, 2022 · 541f0ba · 541f0ba
2 parents cb03e30 + 106950c
commit 541f0ba
Show file tree

Hide file tree

Showing 14 changed files with 188 additions and 274 deletions.
diff --git a/dotcom-rendering/cypress/integration/parallel-1/article.elements.spec.js b/dotcom-rendering/cypress/integration/parallel-1/article.elements.spec.js
@@ -61,7 +61,7 @@ describe('Elements', function () {
 			let hasElementTooWide = false;
 
 			cy.visit(
-				'Article?url=https://www.theguardian.com/commentisfree/2020/jun/30/conservatives-cowboy-builders-boris-johnson',
+				'Article?url=https://www.theguardian.com/politics/2022/jan/21/blackmail-allegations-need-to-be-investigated-says-kwasi-kwarteng',
 			);
 
 			const pageHasXOverflow = (docWidth) => {
@@ -92,22 +92,6 @@ describe('Elements', function () {
 				),
 			);
 		});
-		// Commenting out because this test was failing the build on main on CI
-		// it('should render the instagram embed', function () {
-		// 	// https://www.cypress.io/blog/2020/02/12/working-with-iframes-in-cypress/
-		// 	const getIframeBody = () => {
-		// 		return cy
-		// 			.get('div[data-cy="instagram-embed"] > iframe')
-		// 			.its('0.contentDocument.body')
-		// 			.should('not.be.empty')
-		// 			.then(cy.wrap);
-		// 	};
-		// 	cy.visit(
-		// 		'Article?url=https://www.theguardian.com/media/2018/aug/29/flat-tummy-instagram-women-appetite-suppressant-lollipops',
-		// 	);
-
-		// 	getIframeBody().contains('View More on Instagram');
-		// });
 
 		it('should render the click to view overlay revealing the embed when clicked', function () {
 			const getIframeBody = () => {

diff --git a/dotcom-rendering/package.json b/dotcom-rendering/package.json
@@ -51,7 +51,7 @@
     "@guardian/braze-components": "^5.3.0",
     "@guardian/commercial-core": "^0.32.0",
     "@guardian/consent-management-platform": "^10.1.0",
-    "@guardian/discussion-rendering": "^9.1.0",
+    "@guardian/discussion-rendering": "^9.1.1",
     "@guardian/libs": "^3.4.1",
     "@guardian/shimport": "^1.0.2",
     "@hkdobrev/run-if-changed": "^0.3.1",

diff --git a/dotcom-rendering/src/web/components/App.tsx b/dotcom-rendering/src/web/components/App.tsx
@@ -8,7 +8,6 @@ import { ReaderRevenueLinks } from '@frontend/web/components/ReaderRevenueLinks'
 import { SlotBodyEnd } from '@root/src/web/components/SlotBodyEnd/SlotBodyEnd';
 import { ContributionSlot } from '@frontend/web/components/ContributionSlot';
 import { GetMatchNav } from '@frontend/web/components/GetMatchNav';
-import { Discussion } from '@frontend/web/components/Discussion';
 import { StickyBottomBanner } from '@root/src/web/components/StickyBottomBanner/StickyBottomBanner';
 import { SignInGateSelector } from '@root/src/web/components/SignInGate/SignInGateSelector';
 
@@ -39,7 +38,6 @@ import { decideDisplay } from '@root/src/web/lib/decideDisplay';
 import { decideDesign } from '@root/src/web/lib/decideDesign';
 import { useOnce } from '@root/src/web/lib/useOnce';
 import { initPerf } from '@root/src/web/browser/initPerf';
-import { getUser } from '@root/src/web/lib/getUser';
 
 import { FocusStyleManager } from '@guardian/source-foundations';
 import {
@@ -108,7 +106,6 @@ let renderCount = 0;
 export const App = ({ CAPI, ophanRecord }: Props) => {
 	log('dotcom', `App.tsx render #${(renderCount += 1)}`);
 	const isSignedIn = !!getCookie({ name: 'GU_U', shouldMemoize: true });
-	const [user, setUser] = useState<UserProfile | null>();
 
 	const [brazeMessages, setBrazeMessages] =
 		useState<Promise<BrazeMessagesInterface>>();
@@ -144,23 +141,6 @@ export const App = ({ CAPI, ophanRecord }: Props) => {
 		log('dotcom', 'AB tests initialised');
 	}, [ABTestAPI]);
 
-	useOnce(() => {
-		// useOnce means this code will only run once isSignedIn is defined, and only
-		// run one time
-		if (isSignedIn) {
-			getUser(CAPI.config.discussionApiUrl)
-				.then((theUser) => {
-					if (theUser) {
-						setUser(theUser);
-						log('dotcom', 'State: user set');
-					}
-				})
-				.catch((e) => console.error(`getUser - error: ${e}`));
-		} else {
-			setUser(null);
-		}
-	}, [isSignedIn, CAPI.config.discussionApiUrl]);
-
 	useEffect(() => {
 		incrementAlreadyVisited();
 	}, []);
@@ -929,22 +909,6 @@ export const App = ({ CAPI, ophanRecord }: Props) => {
 					pageViewId={pageViewId}
 				/>
 			</Portal>
-			<HydrateOnce rootId="comments" waitFor={[user]}>
-				<Discussion
-					format={format}
-					discussionApiUrl={CAPI.config.discussionApiUrl}
-					shortUrlId={CAPI.config.shortUrlId}
-					user={user || undefined}
-					discussionD2Uid={CAPI.config.discussionD2Uid}
-					discussionApiClientHeader={
-						CAPI.config.discussionApiClientHeader
-					}
-					enableDiscussionSwitch={CAPI.config.enableDiscussionSwitch}
-					isAdFreeUser={CAPI.isAdFreeUser}
-					shouldHideAds={CAPI.shouldHideAds}
-					beingHydrated={true}
-				/>
-			</HydrateOnce>
 			<Portal rootId="most-viewed-footer">
 				<MostViewedFooter
 					format={format}

diff --git a/dotcom-rendering/src/web/components/Discussion.stories.tsx b/dotcom-rendering/src/web/components/Discussion.stories.tsx
@@ -22,7 +22,6 @@ export const Basic = () => {
 				enableDiscussionSwitch={true}
 				isAdFreeUser={false}
 				shouldHideAds={false}
-				beingHydrated={true}
 			/>
 		</div>
 	);

diff --git a/dotcom-rendering/src/web/components/Discussion.tsx b/dotcom-rendering/src/web/components/Discussion.tsx
@@ -1,22 +1,21 @@
 import { useState, useEffect } from 'react';
 import { css } from '@emotion/react';
 
+import { joinUrl } from '@guardian/libs';
 import { space, from } from '@guardian/source-foundations';
 import { RightColumn } from '@frontend/web/components/RightColumn';
 import { AdSlot } from '@root/src/web/components/AdSlot';
 import { App as Comments } from '@guardian/discussion-rendering';
 
-import { Lazy } from '@frontend/web/components/Lazy';
 import { Flex } from '@frontend/web/components/Flex';
 import { SignedInAs } from '@frontend/web/components/SignedInAs';
 import { ContainerLayout } from '@frontend/web/components/ContainerLayout';
 import { Hide } from '@frontend/web/components/Hide';
 import { getCommentContext } from '@root/src/web/lib/getCommentContext';
-import { joinUrl } from '@root/src/lib/joinUrl';
 import { useDiscussion } from '@root/src/web/lib/useDiscussion';
 import { decidePalette } from '../lib/decidePalette';
 
-type Props = {
+export type Props = {
 	format: ArticleFormat;
 	discussionApiUrl: string;
 	shortUrlId: string;
@@ -26,20 +25,6 @@ type Props = {
 	isAdFreeUser: boolean;
 	shouldHideAds: boolean;
 	user?: UserProfile;
-	// **************************************************************************
-	// beingHydrated?
-	// We use this prop to solve a problem we have with Storybook. If you remove
-	// it then the page will render fine on production but our layout stories
-	// will render two copies of Discussion, side by side. The reason for this
-	// is because we technically server side render these layout stories on the
-	// client, inside the story file itself.
-	//
-	// Yes, it's true, having props purely to solve test implementation problems
-	// is not great. If you feel strongly about this and want to remove this
-	// prop I'm okay with that. If you were able to solve this another way
-	// then thank you!
-	beingHydrated?: boolean;
-	// **************************************************************************
 };
 
 const commentIdFromUrl = () => {
@@ -55,13 +40,12 @@ export const Discussion = ({
 	format,
 	discussionApiUrl,
 	shortUrlId,
-	user,
 	discussionD2Uid,
 	discussionApiClientHeader,
 	enableDiscussionSwitch,
 	isAdFreeUser,
 	shouldHideAds,
-	beingHydrated,
+	user,
 }: Props) => {
 	const [commentPage, setCommentPage] = useState<number>();
 	const [commentPageSize, setCommentPageSize] = useState<25 | 50 | 100>();
@@ -74,7 +58,7 @@ export const Discussion = ({
 	);
 
 	const { commentCount, isClosedForComments } = useDiscussion(
-		joinUrl([discussionApiUrl, 'discussion', shortUrlId]),
+		joinUrl(discussionApiUrl, 'discussion', shortUrlId),
 	);
 
 	const palette = decidePalette(format);
@@ -180,62 +164,29 @@ export const Discussion = ({
 								/>
 							</div>
 						</Hide>
-
-						{beingHydrated && isExpanded && (
-							<Comments
-								user={user}
-								baseUrl={discussionApiUrl}
-								pillar={format.theme}
-								initialPage={commentPage}
-								pageSizeOverride={commentPageSize}
-								isClosedForComments={
-									isClosedForComments ||
-									!enableDiscussionSwitch
-								}
-								orderByOverride={commentOrderBy}
-								shortUrl={shortUrlId}
-								additionalHeaders={{
-									'D2-X-UID': discussionD2Uid,
-									'GU-Client': discussionApiClientHeader,
-								}}
-								expanded={isExpanded}
-								commentToScrollTo={hashCommentId}
-								onPermalinkClick={handlePermalink}
-								apiKey="dotcom-rendering"
-								onExpanded={(value) => {
-									handleExpanded(value);
-								}}
-							/>
-						)}
-
-						{beingHydrated && !isExpanded && (
-							<Lazy margin={300} disableFlexStyles={true}>
-								<Comments
-									user={user}
-									baseUrl={discussionApiUrl}
-									pillar={format.theme}
-									initialPage={commentPage}
-									pageSizeOverride={commentPageSize}
-									isClosedForComments={
-										isClosedForComments ||
-										!enableDiscussionSwitch
-									}
-									orderByOverride={commentOrderBy}
-									shortUrl={shortUrlId}
-									additionalHeaders={{
-										'D2-X-UID': discussionD2Uid,
-										'GU-Client': discussionApiClientHeader,
-									}}
-									expanded={isExpanded}
-									commentToScrollTo={hashCommentId}
-									onPermalinkClick={handlePermalink}
-									apiKey="dotcom-rendering"
-									onExpanded={(value) => {
-										handleExpanded(value);
-									}}
-								/>
-							</Lazy>
-						)}
+						<Comments
+							user={user}
+							baseUrl={discussionApiUrl}
+							pillar={format.theme}
+							initialPage={commentPage}
+							pageSizeOverride={commentPageSize}
+							isClosedForComments={
+								isClosedForComments || !enableDiscussionSwitch
+							}
+							orderByOverride={commentOrderBy}
+							shortUrl={shortUrlId}
+							additionalHeaders={{
+								'D2-X-UID': discussionD2Uid,
+								'GU-Client': discussionApiClientHeader,
+							}}
+							expanded={isExpanded}
+							commentToScrollTo={hashCommentId}
+							onPermalinkClick={handlePermalink}
+							apiKey="dotcom-rendering"
+							onExpanded={(value) => {
+								handleExpanded(value);
+							}}
+						/>
 					</div>
 					<>
 						{!hideAd && (

diff --git a/dotcom-rendering/src/web/components/DiscussionContainer.importable.tsx b/dotcom-rendering/src/web/components/DiscussionContainer.importable.tsx
@@ -0,0 +1,31 @@
+import { getCookie } from '@guardian/libs';
+import { Discussion } from '@root/src/web/components/Discussion';
+import { DiscussionWhenSignedIn } from '@root/src/web/components/DiscussionWhenSignedIn';
+import type { Props as DiscussionProps } from 'src/web/components/Discussion';
+
+/**
+ * DiscussionContainer
+ *
+ * A wrapper component that decides if the user is signed in or not.
+ *
+ * If they
+ * are, it renders DiscussionWhenSignedIn which includes an api call to fetch
+ * the user profile.
+ *
+ * If not, it simply renders Discussion
+ *
+ * We use component composition like this here because you cannoy call react
+ * hooks conditionally and we're using a hook to make the fetch request
+ *
+ * Note. We allow the ...props pattern here because it makes sense when we're
+ * just passing them through
+ *
+ */
+// eslint-disable-next-line react/destructuring-assignment
+export const DiscussionContainer = (props: DiscussionProps) => {
+	const isSignedIn = !!getCookie({ name: 'GU_U', shouldMemoize: true });
+	// eslint-disable-next-line react/jsx-props-no-spreading
+	if (isSignedIn) return <DiscussionWhenSignedIn {...props} />;
+	// eslint-disable-next-line react/jsx-props-no-spreading
+	return <Discussion {...props} />;
+};
diff --git a/dotcom-rendering/src/web/components/DiscussionWhenSignedIn.tsx b/dotcom-rendering/src/web/components/DiscussionWhenSignedIn.tsx
@@ -0,0 +1,19 @@
+import { joinUrl } from '@guardian/libs';
+import { Discussion } from '@root/src/web/components/Discussion';
+import type { Props as DiscussionProps } from 'src/web/components/Discussion';
+import { useApi } from '../lib/useApi';
+
+// eslint-disable-next-line react/destructuring-assignment
+export const DiscussionWhenSignedIn = (props: DiscussionProps) => {
+	const { discussionApiUrl } = props;
+	const { data } = useApi<{ userProfile: UserProfile }>(
+		joinUrl(discussionApiUrl, 'profile/me?strict_sanctions_check=false'),
+		{},
+		{
+			credentials: 'include',
+		},
+	);
+	if (!data) return null;
+	// eslint-disable-next-line react/jsx-props-no-spreading
+	return <Discussion user={data.userProfile} {...props} />;
+};
diff --git a/dotcom-rendering/src/web/layouts/CommentLayout.tsx b/dotcom-rendering/src/web/layouts/CommentLayout.tsx
@@ -33,7 +33,7 @@ import { MobileStickyContainer, AdSlot } from '@root/src/web/components/AdSlot';
 import { Border } from '@root/src/web/components/Border';
 import { GridItem } from '@root/src/web/components/GridItem';
 import { AgeWarning } from '@root/src/web/components/AgeWarning';
-import { Discussion } from '@frontend/web/components/Discussion';
+import { DiscussionContainer } from '@root/src/web/components/DiscussionContainer.importable';
 
 import { buildAdTargeting } from '@root/src/lib/ad-targeting';
 import { parse } from '@frontend/lib/slot-machine-flags';
@@ -641,19 +641,22 @@ export const CommentLayout = ({
 
 				{!isPaidContent && showComments && (
 					<ElementContainer sectionId="comments" element="aside">
-						<Discussion
-							discussionApiUrl={CAPI.config.discussionApiUrl}
-							shortUrlId={CAPI.config.shortUrlId}
-							format={format}
-							discussionD2Uid={CAPI.config.discussionD2Uid}
-							discussionApiClientHeader={
-								CAPI.config.discussionApiClientHeader
-							}
-							enableDiscussionSwitch={false}
-							isAdFreeUser={CAPI.isAdFreeUser}
-							shouldHideAds={CAPI.shouldHideAds}
-							beingHydrated={false}
-						/>
+						<Island clientOnly={true} deferUntil="visible">
+							<DiscussionContainer
+								discussionApiUrl={CAPI.config.discussionApiUrl}
+								shortUrlId={CAPI.config.shortUrlId}
+								format={format}
+								discussionD2Uid={CAPI.config.discussionD2Uid}
+								discussionApiClientHeader={
+									CAPI.config.discussionApiClientHeader
+								}
+								enableDiscussionSwitch={
+									CAPI.config.switches.enableDiscussionSwitch
+								}
+								isAdFreeUser={CAPI.isAdFreeUser}
+								shouldHideAds={CAPI.shouldHideAds}
+							/>
+						</Island>
 					</ElementContainer>
 				)}