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

fix(meta_tags): add special case for dealing with json-ld scripts #35

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bertyhell
Copy link

closes: #34

Removes all instances of <script type="application/ld+json"> before adding a new one

before this PR:
image

after this PR:
image

@bertyhell
Copy link
Author

@s-yadav any chance to merge this?

@oles-bolotniuk
Copy link

@s-yadav ping

@s-yadav
Copy link
Owner

s-yadav commented Oct 14, 2020

@oles-bolotniuk @bertyhell Can't there be multiple json-ld scripts in a page?
I guess this should be handled with script tag check and id attribute.

If there can be only one json-ld script in a page, then the changes looks good.

@bertyhell
Copy link
Author

It looks like multiple json-ld scripts are allowed according to the spec:
https://w3c.github.io/json-ld-syntax/#embedding-json-ld-in-html-documents

@alexander-morgunov
Copy link

@bertyhell the problem is that in my case, I need to update tag while navigate to another page. In current solution - I'll get new tag on each route change, without removing old ones.

@s-yadav
Copy link
Owner

s-yadav commented Oct 14, 2020

@bertyhell I guess then we can handle it based on id. The library already checks for id to identify uniqueness. But the script is not in the inclusion list. We can include script tag as well for this.

On other note, only non-js scripts make sense to be kept inside meta tags. For normal JavaScript scripts once the methods/variables are added to global scope it can't be removed so it doesn't make to handle update for those.

@bertyhell
Copy link
Author

bertyhell commented Oct 19, 2020

for anyone blocked by this issue, for now you can just include your own component with your own rules for deleting other tags:

import { FunctionComponent } from 'react';

export interface JsonLdProps {
	url: string;
	title?: string;
	description?: string | null;
}

const JsonLd: FunctionComponent<JsonLdProps> = ({
	url,
	title,
	description,
}) => {
	document
		.querySelectorAll('script[type="application/ld+json"]')
		.forEach((script) => script.remove());

	const info = {
		'@context': 'https://schema.org',
		'@type': 'Article',
		mainEntityOfPage: {
			'@type': 'WebPage',
			'@id': url,
		},
		headline: title || '',
		description: description || '',
	};

	const scriptElem = document.createElement('script');
	scriptElem.setAttribute('type', 'application/ld+json');
	scriptElem.innerHTML = JSON.stringify(info, null, 2);
	document.head.append(scriptElem);
	return null;
};

export default JsonLd;

The relevant line for deleting other scripts is:

	document
		.querySelectorAll('script[type="application/ld+json"]')
		.forEach((script) => script.remove());

you could for instance only delete tags with a specific id:

	document
		.querySelectorAll('script[type="application/ld+json"][id="mySpecificId"]')
		.forEach((script) => script.remove());

s-yadav added a commit that referenced this pull request Nov 25, 2020
- Bump react dependency to 16. #37
- Uniquely identify any elements inside MetaTags by id. Fix for #35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Script tag JSON-LD render twice!
4 participants