Skip to content
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

Lite Site: Remove data-react-helmet="true" attributes from all tags within the html <head> tag #12350

Draft
wants to merge 14 commits into
base: latest
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3,847 changes: 3,847 additions & 0 deletions data/gahuza/live/c2dl5y6dgrzt.json

Large diffs are not rendered by default.

11 changes: 11 additions & 0 deletions src/app/routes/utils/constructPageFetchUrl/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,17 +96,28 @@ describe('constructPageFetchUrl', () => {
${HOME_PAGE} | ${null} | ${null} | ${'test'} | ${'c0000000000t'} | ${'https://mock-bff-path/?id=cl13j7792ljt&service=ukrainian&pageType=home&serviceEnv=test'}
${HOME_PAGE} | ${null} | ${null} | ${'live'} | ${'c0000000000t'} | ${'https://mock-bff-path/?id=c3eg5kglplrt&service=ukrainian&pageType=home&serviceEnv=live'}
${LIVE_PAGE} | ${null} | ${null} | ${'local'} | ${'c0000000000t'} | ${'http://localhost/api/local/ukrainian/live/c0000000000t'}
${LIVE_PAGE} | ${null} | ${null} | ${'local'} | ${'c0000000000t.lite'} | ${'http://localhost/api/local/ukrainian/live/c0000000000t'}
${LIVE_PAGE} | ${'serbian'} | ${'cyr'} | ${'local'} | ${'c0000000000t'} | ${'http://localhost/api/local/serbian/live/c0000000000t/cyr'}
${LIVE_PAGE} | ${'serbian'} | ${'cyr'} | ${'local'} | ${'c0000000000t'} | ${'http://localhost/api/local/serbian/live/c0000000000t/cyr'}
${LIVE_PAGE} | ${null} | ${null} | ${'test'} | ${'c0000000000t'} | ${'https://mock-bff-path/?id=c0000000000t&service=ukrainian&pageType=live&serviceEnv=test'}
${LIVE_PAGE} | ${null} | ${null} | ${'test'} | ${'c0000000000t.lite'} | ${'https://mock-bff-path/?id=c0000000000t&service=ukrainian&pageType=live&serviceEnv=test'}
${LIVE_PAGE} | ${'zhongwen'} | ${'trad'} | ${'test'} | ${'c0000000000t'} | ${'https://mock-bff-path/?id=c0000000000t&service=zhongwen&pageType=live&variant=trad&serviceEnv=test'}
${LIVE_PAGE} | ${'zhongwen'} | ${'trad'} | ${'test'} | ${'c0000000000t.lite'} | ${'https://mock-bff-path/?id=c0000000000t&service=zhongwen&pageType=live&variant=trad&serviceEnv=test'}
${LIVE_PAGE} | ${null} | ${null} | ${'live'} | ${'c0000000000t'} | ${'https://mock-bff-path/?id=c0000000000t&service=ukrainian&pageType=live&serviceEnv=live'}
${LIVE_PAGE} | ${null} | ${null} | ${'live'} | ${'c0000000000t.lite'} | ${'https://mock-bff-path/?id=c0000000000t&service=ukrainian&pageType=live&serviceEnv=live'}
${LIVE_PAGE} | ${'serbian'} | ${'cyr'} | ${'live'} | ${'c0000000000t'} | ${'https://mock-bff-path/?id=c0000000000t&service=serbian&pageType=live&variant=cyr&serviceEnv=live'}
${LIVE_PAGE} | ${'serbian'} | ${'cyr'} | ${'live'} | ${'c0000000000t.lite'} | ${'https://mock-bff-path/?id=c0000000000t&service=serbian&pageType=live&variant=cyr&serviceEnv=live'}
${LIVE_PAGE} | ${'arabic'} | ${null} | ${'local'} | ${'67574192'} | ${'http://localhost/api/local/arabic/live/67574192'}
${LIVE_PAGE} | ${'arabic'} | ${null} | ${'local'} | ${'67574192.lite'} | ${'http://localhost/api/local/arabic/live/67574192'}
${LIVE_PAGE} | ${'zhongwen'} | ${'trad'} | ${'local'} | ${'uk-69168527'} | ${'http://localhost/api/local/zhongwen/live/uk-69168527/trad'}
${LIVE_PAGE} | ${'zhongwen'} | ${'trad'} | ${'local'} | ${'uk-69168527.lite'} | ${'http://localhost/api/local/zhongwen/live/uk-69168527/trad'}
${LIVE_PAGE} | ${'arabic'} | ${null} | ${'test'} | ${'67574192'} | ${'https://mock-bff-path/?id=%2Farabic%2Flive%2F67574192&service=arabic&pageType=live&serviceEnv=test'}
${LIVE_PAGE} | ${'arabic'} | ${null} | ${'test'} | ${'67574192.lite'} | ${'https://mock-bff-path/?id=%2Farabic%2Flive%2F67574192&service=arabic&pageType=live&serviceEnv=test'}
${LIVE_PAGE} | ${'serbian'} | ${'lat'} | ${'test'} | ${'media-23179005'} | ${'https://mock-bff-path/?id=%2Fserbian%2Flat%2Flive%2Fmedia-23179005&service=serbian&pageType=live&variant=lat&serviceEnv=test'}
${LIVE_PAGE} | ${'arabic'} | ${null} | ${'live'} | ${'67574192'} | ${'https://mock-bff-path/?id=%2Farabic%2Flive%2F67574192&service=arabic&pageType=live&serviceEnv=live'}
${LIVE_PAGE} | ${'arabic'} | ${null} | ${'live'} | ${'67574192.lite'} | ${'https://mock-bff-path/?id=%2Farabic%2Flive%2F67574192&service=arabic&pageType=live&serviceEnv=live'}
${LIVE_PAGE} | ${'zhongwen'} | ${'trad'} | ${'live'} | ${'uk-69168527'} | ${'https://mock-bff-path/?id=%2Fzhongwen%2Ftrad%2Flive%2Fuk-69168527&service=zhongwen&pageType=live&variant=trad&serviceEnv=live'}
${LIVE_PAGE} | ${'zhongwen'} | ${'trad'} | ${'live'} | ${'uk-69168527.lite'} | ${'https://mock-bff-path/?id=%2Fzhongwen%2Ftrad%2Flive%2Fuk-69168527&service=zhongwen&pageType=live&variant=trad&serviceEnv=live'}
${TOPIC_PAGE} | ${null} | ${null} | ${'local'} | ${'/ukrainian/topics/c0000000000t'} | ${'http://localhost/ukrainian/topics/c0000000000t'}
${TOPIC_PAGE} | ${null} | ${null} | ${'test'} | ${'/ukrainian/topics/c0000000000t'} | ${'https://mock-bff-path/?id=c0000000000t&service=ukrainian&pageType=topic&serviceEnv=test'}
${TOPIC_PAGE} | ${null} | ${null} | ${'live'} | ${'/ukrainian/topics/c0000000000t'} | ${'https://mock-bff-path/?id=c0000000000t&service=ukrainian&pageType=topic&serviceEnv=live'}
Expand Down
11 changes: 7 additions & 4 deletions src/app/routes/utils/constructPageFetchUrl/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import {
import parseAvRoute from '../parseAvRoute';

const removeLeadingSlash = (path: string) => path?.replace(/^\/+/g, '');
const removeAmp = (path: string) => path.split('.')[0];
const removeSuffix = (path: string) => path.split('.')[0];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this perhaps be renamed to removePlatform instead?

const getArticleId = (path: string) => path.match(/(c[a-zA-Z0-9]{10,}o)/)?.[1];
const getCpsId = (path: string) => removeLeadingSlash(path);
const getTVAudioId = (path: string) => removeLeadingSlash(path);
Expand Down Expand Up @@ -155,7 +155,7 @@ const getId = ({ pageType, service, variant, env }: GetIdProps) => {
getIdFunction = () => null;
break;
}
return pipe(getUrlPath, removeAmp, getIdFunction);
return pipe(getUrlPath, removeSuffix, getIdFunction);
};

export interface UrlConstructParams {
Expand Down Expand Up @@ -257,9 +257,12 @@ const constructPageFetchUrl = ({
const variantPath = variant ? `/${variant}` : '';
const host = `http://${process.env.HOSTNAME || 'localhost'}`;
const port = process.env.PORT ? `:${process.env.PORT}` : '';
// pathname is the ID of the Live page without /service/live/, and supports both Tipo & CPS IDs

let liveId = id;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to add a comment here?

if (!isTipoIdCheck(id)) liveId = removeSuffix(pathname);

fetchUrl = Url(
`${host}${port}/api/local/${service}/live/${pathname}${variantPath}`,
`${host}${port}/api/local/${service}/live/${liveId}${variantPath}`,
);
break;
}
Expand Down
10 changes: 10 additions & 0 deletions src/integration/common/liteSite.lite.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,14 @@ export default () => {
expect(liteSiteCta).toMatchSnapshot();
});
});

describe('data-react-helmet attribute', () => {
const tagsWithDataReactHelmet = document.querySelectorAll(
'[data-react-helmet="true"]',
);

it('should not be in the document', () => {
expect(tagsWithDataReactHelmet).toHaveLength(0);
});
});
};
12 changes: 12 additions & 0 deletions src/integration/pages/homePage/gahuzaLiteSite/lite.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/**
* @service gahuza
* @pathname /gahuza
*/

import { runLiteSiteTests } from '../../../common';

describe('Lite Site', () => {
describe(pageType, () => {
runLiteSiteTests();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import React from 'react';
import serialiseForScript from '#app/lib/utilities/serialiseForScript';
import removeDataReactHelmetAttribute from '.';

describe('removeDataReactHelmetAttribute', () => {
it('removes data-react-helmet from meta tags', () => {
const metaTags = [
<meta data-react-helmet="true" name="name1" content="content1" />,
<meta data-react-helmet="true" name="name2" content="content2" />,
];

expect(removeDataReactHelmetAttribute(metaTags)).toStrictEqual([
<meta name="name1" content="content1" />,
<meta name="name2" content="content2" />,
]);
});

it('removes data-react-helmet from link tags', () => {
const linkTags = [
<link
data-react-helmet="true"
rel="canonical"
href="https://www.bbc.com/pidgin"
/>,
<link
data-react-helmet="true"
rel="icon"
href="https://www.bbc.com/pidgin"
/>,
];

expect(removeDataReactHelmetAttribute(linkTags)).toStrictEqual([
<link rel="canonical" href="https://www.bbc.com/pidgin" />,
<link rel="icon" href="https://www.bbc.com/pidgin" />,
]);
});

it('removes data-react-helmet from script tags', () => {
const scriptTags = [
<script data-react-helmet="true" type="text/javascript">
console.log(`Hello World`);
</script>,
<script data-react-helmet="true" type="application/ld+json">
{serialiseForScript({ key: 'value' })}
</script>,
];

expect(removeDataReactHelmetAttribute(scriptTags)).toStrictEqual([
<script type="text/javascript">console.log(`Hello World`);</script>,
<script type="application/ld+json">
{serialiseForScript({ key: 'value' })}
</script>,
]);
});

it('removes data-react-helmet from title tag', () => {
const title = <title data-react-helmet="true">Title</title>;

expect(removeDataReactHelmetAttribute(title)).toStrictEqual(
<title>Title</title>,
);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { ReactElement } from 'react';

const getPropsExcludingDataReactHelmet = (props: object) => {
return Object.fromEntries(
Object.entries(props).filter(([key]) => key !== 'data-react-helmet'),
);
};

export default (tags: ReactElement | ReactElement[]) => {
if (Array.isArray(tags)) {
return tags.map(tag => {
return {
...tag,
props: getPropsExcludingDataReactHelmet(tag.props),
};
});
}

return {
...tags,
props: getPropsExcludingDataReactHelmet(tags.props),
};
};
6 changes: 1 addition & 5 deletions src/server/Document/__snapshots__/component.test.jsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -187,18 +187,14 @@ exports[`Document Component should render LITE version correctly 1`] = `
content="none"
name="robots"
/>
<title
data-react-helmet="true"
>
<title>
Test title
</title>
<meta
content="width=device-width, initial-scale=1, minimum-scale=1"
data-react-helmet="true"
name="viewport"
/>
<link
data-react-helmet="true"
href="https://www.bbc.com/news/articles/c6v11qzyv8po"
rel="canonical"
/>
Expand Down
17 changes: 12 additions & 5 deletions src/server/Document/component.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
/* eslint-disable react/no-danger */
import React from 'react';
import React, { ReactElement } from 'react';
import { EmotionCritical } from '@emotion/server/create-instance';

import { HelmetData } from 'react-helmet';
import LiteRenderer from './Renderers/LiteRenderer';
import CanonicalRenderer from './Renderers/CanonicalRenderer';
import AmpRenderer from './Renderers/AmpRenderer';
import litePageTransforms from './Renderers/litePageTransforms';
import removeDataReactHelmetAttribute from './Renderers/litePageTransforms/removeDataReactHelmetAttribute';

type Props = {
app: EmotionCritical;
Expand Down Expand Up @@ -48,12 +49,18 @@ const Document = ({
dangerouslySetInnerHTML={{ __html: litePageTransforms(html) }}
/>
}
helmetLinkTags={helmetLinkTags}
helmetMetaTags={helmetMetaTags}
helmetScriptTags={helmetScriptTags}
helmetLinkTags={
removeDataReactHelmetAttribute(helmetLinkTags) as ReactElement
}
helmetMetaTags={
removeDataReactHelmetAttribute(helmetMetaTags) as ReactElement
}
helmetScriptTags={
removeDataReactHelmetAttribute(helmetScriptTags) as ReactElement
}
htmlAttrs={htmlAttrs}
styles={css}
title={title}
title={removeDataReactHelmetAttribute(title) as ReactElement}
/>
);
case isAmp:
Expand Down
12 changes: 12 additions & 0 deletions ws-nextjs-app/integration/pages/live/gahuzaLiteSite/lite.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/**
* @service gahuza
* @pathname /gahuza/live/c2dl5y6dgrzt
*/

import { runLiteSiteTests } from '../../../../../src/integration/common';

describe('Lite Site', () => {
describe(pageType, () => {
runLiteSiteTests();
});
});
15 changes: 11 additions & 4 deletions ws-nextjs-app/pages/_document.page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {

import LiteRenderer from '#server/Document/Renderers/LiteRenderer';
import litePageTransforms from '#server/Document/Renderers/litePageTransforms';
import removeDataReactHelmetAttribute from '#server/Document/Renderers/litePageTransforms/removeDataReactHelmetAttribute';
import sendCustomMetric from '#server/utilities/customMetrics';
import { NON_200_RESPONSE } from '#server/utilities/customMetrics/metrics.const';

Expand Down Expand Up @@ -143,12 +144,18 @@ export default class AppDocument extends Document<DocProps> {
return (
<LiteRenderer
bodyContent={<Main />}
helmetLinkTags={helmetLinkTags}
helmetMetaTags={helmetMetaTags}
helmetScriptTags={helmetScriptTags}
helmetLinkTags={
removeDataReactHelmetAttribute(helmetLinkTags) as ReactElement
}
helmetMetaTags={
removeDataReactHelmetAttribute(helmetMetaTags) as ReactElement
}
helmetScriptTags={
removeDataReactHelmetAttribute(helmetScriptTags) as ReactElement
}
htmlAttrs={htmlAttrs}
styles={css}
title={title}
title={removeDataReactHelmetAttribute(title) as ReactElement}
/>
);
default:
Expand Down
Loading