fix(core): give the JSON-LD nodes an @id so they can be referenced - #3019
fix(core): give the JSON-LD nodes an @id so they can be referenced#3019marcin-misiewicz-pl wants to merge 2 commits into
Conversation
`BlogPosting`, its `publisher` and `WebSite` were emitted without `@id`, which
makes each an anonymous node: nothing can point at it, and a fuller description
of the same thing published alongside it stays a separate entity.
The visible consequence is on article pages. A site that publishes an
Organization graph — from a plugin, or hand-written in a template — ends up with
two organisations: the rich one, and core's `{ "@type": "Organization", name }`
sitting inside `publisher`. A consumer has no way to merge them, so the article's
publisher is the one carrying nothing but a name, and every property the site
took care to state is attached to something the article does not reference.
`WebSite` has the same problem from the other direction: with no `@id` there is
nowhere to attach a `potentialAction`, so adding a sitelinks SearchAction means
emitting a second WebSite node and claiming the site is two sites.
Three ids, chosen to be joinable:
- `BlogPosting` -> `<canonical>#article`. A fragment rather than the bare
canonical, because `mainEntityOfPage` already identifies the WebPage by that
IRI and reusing it would state that the article and its page are one thing.
- `publisher` -> `<origin>/#organization`, the conventional form, and the one
already produced by `url.replace(/\/$/, "") + "/#organization"`. The slash
before the fragment is load-bearing: `https://x.com#organization` is a
different IRI, and a mismatch does not error — it publishes two organisations.
- `WebSite` -> `<origin>#website`.
`publisher` keeps its `@type` and `name`. Emitting a bare `{ "@id": … }` would
leave a dangling reference on every site that publishes no Organization graph,
which is worse than the anonymous node it replaces. Nothing changes for those
sites.
`buildWebSiteJsonLd` had the origin-resolution chain inline; `buildBlogPostingJsonLd`
now needs the same answer, so it moves to a shared helper. Two copies would
eventually disagree, and for a value used to build an `@id` that means silently
publishing two entities. Deliberately not `resolveSiteOrigin()` from
`absolute-url.ts`: that gives `SiteSettings.url` precedence over `page.siteUrl`,
which would change which origin these graphs carry — plausibly an improvement,
but a behaviour change that does not belong in a fix about node identity.
🦋 Changeset detectedLatest commit: 3254c15 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
All contributors have signed the CLA ✍️ ✅ |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR addresses a real JSON-LD gap: anonymous BlogPosting, publisher, and WebSite nodes cannot be referenced by plugins or by-site markup, so richer Organization/WebSite graphs emitted alongside them fail to merge. The approach is sound — giving each node a stable @id and keeping the publisher self-describing is the right fix, and hoisting the origin resolution into a shared helper prevents the two graphs from drifting apart.
I read the diff, the full packages/core/src/page/jsonld.ts, the new tests in page-seo.test.ts, EmDashHead.astro, absolute-url.ts, AGENTS.md, and .changeset/README.md.
Two issues need fixing before this is clean:
-
AGENTS.md comment violations. The new JSDoc for
siteOriginand the inline//comments above each@idare written as PR rationale — they narrate the change, justify the decision not to useresolveSiteOrigin(), and explain rejected alternatives. AGENTS.md's Comments section prohibits exactly this kind of prose. This is a maintenance issue because the comments will be stale the moment the surrounding code evolves. -
siteOrigindoes not normalizepage.siteUrl. Whenpage.siteUrlis configured with a trailing slash (e.g.https://example.com/, a common value fromAstro.site),siteOriginreturns it verbatim and the publisher@idbecomeshttps://example.com//#organization. That is a different IRI than thehttps://example.com/#organizationform the changeset tells users to emit, so it silently re-introduces the two-organisation problem this PR is fixing. The helper should parsepage.siteUrlas a URL and extract the origin when possible, keeping the existing precedence but guaranteeing the canonical ID form.
I also left a suggestion to consider using the same /#<fragment pattern for WebSite as for Organization, so plugin authors have one rule to follow when extending the site graph.
| /** | ||
| * The site's public origin, as the graphs below refer to it. | ||
| * | ||
| * Lifted out of `buildWebSiteJsonLd` unchanged, because `buildBlogPostingJsonLd` | ||
| * now needs the same answer and two copies of this chain would eventually | ||
| * disagree — which, for values used to build an `@id`, means silently | ||
| * publishing two entities instead of one. | ||
| * | ||
| * Deliberately NOT `resolveSiteOrigin()` from `absolute-url.ts`: that gives | ||
| * `SiteSettings.url` precedence over `page.siteUrl`, which would change which | ||
| * origin these graphs carry. That may well be the better order, but it is a | ||
| * behaviour change and does not belong in a fix about node identity. | ||
| */ |
There was a problem hiding this comment.
[needs fixing] The siteOrigin JSDoc is written as PR narrative, not as a comment for future readers of the code. It explains why the helper was extracted from buildWebSiteJsonLd, why resolveSiteOrigin() was rejected, and frames that rejection as a design justification (Deliberately NOT, That may well be the better order, but...).
AGENTS.md's Comments section forbids comments that restate the change, address the reviewer, justify decisions, or narrate rejected alternatives. Replace this with a short description of the helper's contract (or omit it), and keep the precedence rationale in the commit message.
| /** | |
| * The site's public origin, as the graphs below refer to it. | |
| * | |
| * Lifted out of `buildWebSiteJsonLd` unchanged, because `buildBlogPostingJsonLd` | |
| * now needs the same answer and two copies of this chain would eventually | |
| * disagree — which, for values used to build an `@id`, means silently | |
| * publishing two entities instead of one. | |
| * | |
| * Deliberately NOT `resolveSiteOrigin()` from `absolute-url.ts`: that gives | |
| * `SiteSettings.url` precedence over `page.siteUrl`, which would change which | |
| * origin these graphs carry. That may well be the better order, but it is a | |
| * behaviour change and does not belong in a fix about node identity. | |
| */ | |
| /** | |
| * Site origin to use for JSON-LD node identifiers. | |
| * | |
| * `page.siteUrl` wins over `page.url` so IDs stay stable when a theme | |
| * overrides the public origin. Falls back to the raw canonical or URL | |
| * only when neither parses as a URL. | |
| */ | |
| function siteOrigin(page: PublicPageContext): string { |
| // A fragment, not the bare canonical: `mainEntityOfPage` below already | ||
| // identifies the WebPage by that IRI, and reusing it would state that the | ||
| // article and the page it sits on are the same thing. |
There was a problem hiding this comment.
[needs fixing] These // comments explain the PR design rationale (why the article @id is a fragment, why it must not reuse the canonical, what would go wrong otherwise). AGENTS.md permits comments only when the code is genuinely unclear or the reason is non-obvious. The object key "@id" itself is not unclear; the JSON-LD nuance is better captured by a short docstring or left to the changeset. Remove the narrative inline comments.
| // Identified, but still self-describing. `@id` lets a fuller Organization | ||
| // graph — from a plugin, or hand-written in a template — merge into this | ||
| // node instead of standing beside it as a second, competing organisation. | ||
| // `@type` and `name` stay so that a site publishing no such graph is left | ||
| // with a complete node rather than a dangling reference. |
There was a problem hiding this comment.
[needs fixing] Same issue as the article @id comments: this block justifies the publisher shape to the reader (Identified, but still self-describing, explains what a bare @id would do, restates why @type and name remain). This is PR/changeset prose, not a code comment. Remove or collapse to a single short phrase if the merging behavior is non-obvious without it.
| // So a plugin can add `potentialAction` (a sitelinks SearchAction, say) | ||
| // by emitting a node under the same `@id`, rather than a second WebSite. |
There was a problem hiding this comment.
[needs fixing] This inline comment also explains the PR rationale (why @id enables plugin extensions) rather than a non-obvious invariant. Remove the narrative comment.
| * behaviour change and does not belong in a fix about node identity. | ||
| */ | ||
| function siteOrigin(page: PublicPageContext): string { | ||
| if (page.siteUrl) return page.siteUrl; |
There was a problem hiding this comment.
[needs fixing] page.siteUrl is returned verbatim, but it is commonly set from user/theme configuration and can carry a trailing slash or path (e.g. https://example.com/). When that happens, the publisher @id becomes https://example.com//#organization, which is not the <origin>/#organization IRI the changeset guarantees and will not merge with a plugin Organization graph.
Parse page.siteUrl through new URL(...).origin when possible so IDs are always canonical origins, matching the helper's name. Keep the fallback to the raw string only when parsing fails.
| if (page.siteUrl) return page.siteUrl; | |
| function siteOrigin(page: PublicPageContext): string { | |
| if (page.siteUrl) { | |
| try { | |
| return new URL(page.siteUrl).origin; | |
| } catch { | |
| return page.siteUrl; | |
| } | |
| } | |
| try { | |
| return new URL(page.url).origin; | |
| } catch { | |
| return page.canonical || page.url; | |
| } | |
| } |
| "@type": "WebSite", | ||
| // So a plugin can add `potentialAction` (a sitelinks SearchAction, say) | ||
| // by emitting a node under the same `@id`, rather than a second WebSite. | ||
| "@id": `${siteUrl}#website`, |
There was a problem hiding this comment.
[suggestion] Organization uses ${siteOrigin(page)}/#organization while WebSite uses ${siteUrl}#website (no slash before the fragment). For consistency with schema.org conventions and the changeset's own emphasis on the exact IRI form, consider making WebSite ${siteUrl}/#website as well, so plugins only have one rule when extending either node.
github-actions
Bot
added
review/awaiting-author
`siteOrigin` returned `page.siteUrl` verbatim. That value comes from user or theme configuration and commonly carries a trailing slash — `Astro.site` is usually written that way — so the publisher id became `https://example.com//#organization`. A doubled slash is a different IRI, which is exactly the two-organisation problem this branch set out to fix, reintroduced silently and only on the sites that configure an origin at all. Parsed through `new URL(...).origin` now, falling back to the raw string only when it does not parse, so the helper returns what its name promises. Covered by a test that pins the slashed form. `WebSite` moves to `<origin>/#website`, matching the organisation's fragment form, so a plugin extending either node follows one rule rather than two. Also strips the comments added with the previous commit. They narrated the change, justified the choice against `resolveSiteOrigin()`, and described alternatives that were considered and rejected — all of which AGENTS.md excludes from comments, and all of which is already in the commit message and the PR where it belongs.
|
Thanks — both findings were right, and the second was a real defect.
Comments. Removed. They were PR rationale in code: they justified the choice against
The CLA is still outstanding and I will sign it separately. |
|
I have read the CLA Document and I hereby sign the CLA |
github-actions
Bot
added
review/needs-rereview
1 participant
What does this PR do?
Core emits
BlogPosting, itspublisher, andWebSitewithout an@id. In JSON-LD that makes each an anonymous node: nothing can point at it, and a fuller description of the same thing published alongside it stays a separate entity instead of merging into one.The visible consequence is on article pages. A site that publishes an Organization graph — from a plugin, or hand-written in a template — ends up with two organisations: the rich one, and core's
{ "@type": "Organization", name }insidepublisher. A consumer cannot merge them, so the article's publisher is the node carrying nothing but a name, and every property the site took care to state is attached to something the article does not reference.WebSitehas the same problem from the other side: with no@idthere is nowhere to attach apotentialAction, so adding a sitelinksSearchActionmeans emitting a secondWebSitenode and claiming the site is two sites.This adds three ids, chosen to be joinable:
@idBlogPosting<canonical>#article— a fragment, becausemainEntityOfPagealready identifies the WebPage by the bare canonical and reusing it would state that the article and its page are one thingpublisher<origin>/#organization— the conventional form, and the oneurl.replace(/\/$/, "") + "/#organization"already producesWebSite<origin>#websiteThe slash before the fragment is load-bearing.
https://example.com#organizationandhttps://example.com/#organizationare different IRIs, and a mismatch does not error — it publishes two organisations instead of one.publisherkeeps its@typeandname. A bare{ "@id": … }would leave a dangling reference on every site that publishes no Organization graph, which is worse than the anonymous node it replaces. Nothing changes for those sites.buildWebSiteJsonLdhad the origin-resolution chain inline andbuildBlogPostingJsonLdnow needs the same answer, so it moves to a shared helper — two copies would eventually disagree, and for a value used to build an@idthat means silently publishing two entities. Deliberately notresolveSiteOrigin()fromabsolute-url.ts: that givesSiteSettings.urlprecedence overpage.siteUrl, which would change which origin these graphs carry. Plausibly an improvement, but a behaviour change that does not belong in a fix about node identity.Closes #
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
Not applicable — no UI change.
Four tests added to
packages/core/tests/unit/plugins/page-seo.test.ts. Three fail before the change; the fourth guards against the regression the obvious fix would introduce (a bare@idpublisher losing its@type/name), and passes both before and after.Full package suite: