-
Notifications
You must be signed in to change notification settings - Fork 181
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
MWPW-133754 - Support structured data API for SEO #1115
Changes from 7 commits
7839333
42f5179
c2f9823
0da1656
993d1c3
88ac2f5
c8a0557
40dc3fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,14 +21,45 @@ export async function getVideoObject(url, seotechAPIUrl) { | |
return body.videoObject; | ||
} | ||
|
||
export default async function appendVideoObjectScriptTag(url, { createTag, getConfig }) { | ||
export async function getStructuredData(url, sheetUrl, seotechAPIUrl) { | ||
const apiUrl = new URL(seotechAPIUrl); | ||
apiUrl.pathname = '/api/v1/web/seotech/getStructuredData'; | ||
apiUrl.searchParams.set('url', url); | ||
if (sheetUrl) { | ||
apiUrl.searchParams.set('sheetUrl', sheetUrl); | ||
} | ||
const resp = await fetch(apiUrl.href, { headers: { 'Content-Type': 'application/json' } }); | ||
const body = await resp?.json(); | ||
if (!resp.ok) { | ||
throw new Error(`Failed to fetch structured data: ${body?.error}`); | ||
} | ||
return body.objects; | ||
} | ||
|
||
export async function appendScriptTag({ locationUrl, getMetadata, createTag, getConfig }) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just import There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is convention of similar features as it prevents import cycle. It also makes testing a little easier |
||
const windowUrl = new URL(locationUrl); | ||
const seotechAPIUrl = getConfig()?.env?.name === 'prod' | ||
? SEOTECH_API_URL_PROD : SEOTECH_API_URL_STAGE; | ||
try { | ||
const obj = await getVideoObject(url, seotechAPIUrl); | ||
|
||
const append = (obj) => { | ||
const script = createTag('script', { type: 'application/ld+json' }, JSON.stringify(obj)); | ||
document.head.append(script); | ||
} catch (e) { | ||
logError(e.message); | ||
}; | ||
|
||
const prs = []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Promises, I guess There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Does this repo prefer I spell these vars out? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
if (getMetadata('seotech-structured-data') === 'on') { | ||
const pageUrl = `${windowUrl.origin}${windowUrl.pathname}`; | ||
const sheetUrl = (new URLSearchParams(windowUrl.search)).get('seotech-sheet-url') || getMetadata('seotech-sheet-url'); | ||
prs.push(getStructuredData(pageUrl, sheetUrl, seotechAPIUrl) | ||
.then((r) => r.forEach((obj) => append(obj))) | ||
.catch((e) => logError(e.message))); | ||
} | ||
if (getMetadata('seotech-video-url')) { | ||
prs.push(getVideoObject(getMetadata('seotech-video-url'), seotechAPIUrl) | ||
.then((r) => append(r)) | ||
.catch((e) => logError(e.message))); | ||
} | ||
return Promise.all(prs); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a need to further chain this in the calling function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes? Caller is |
||
} | ||
|
||
export default appendScriptTag; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also contain the
sheetUrl
so the Splunk messages are clear as to which sheet failed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sheetUrl
is used by the service, so if there is a problem with it then the URL should be returned inbody.error
msg. Will fix api to do this.