fix: Add is404 flag for 404ed routes

[Swanand01]

What

This PR adds the is404 flag for 404ed routes

Why

Currently, when templateByUri{} resolves to a non-existent WordPress route, the 404 template gets returned, but since theres a template to return, the status code presents as a Success, instead of properly indicating (to SEO, crawlers, userland handlers) that there is no WP-route at that page.

Related Issue(s):

• https://github.com/rtCamp/headless/issues/451
• Ensure 404-ed routes return the correct HTTP Status codes #145

How

parseQueryResult in packages/query/src/utils/parse-template.ts is refactored to also parse out the flag is404.

Testing Instructions

Screenshots

Additional Info

Checklist

• I have read the Contribution Guidelines.
• My code is tested to the best of my abilities.
• My code passes all lints (ESLint, tsc, prettier etc.).
• My code has detailed inline documentation.
• I have added unit tests to verify the code works as intended.
• I have updated the project documentation as needed.
• I have added a changeset for this PR using npm run changeset.

[changeset-bot]

🦋 Changeset detected

Latest commit: 680b91d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@snapwp/query	Patch

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

[Swanand01]

@justlevine This here is a quick fix as recommended by @ayushnirwal:
#145 (comment)

[coveralls]

Pull Request Test Coverage Report for Build 14464649106

Details

• 1 of 2 (50.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 53.748%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/next/src/template-renderer/index.tsx	0	1	0.0%

Totals
Change from base Build 14463366481:	0.1%
Covered Lines:	400
Relevant Lines:	661

---

💛 - Coveralls

[justlevine]

LGTM with some small notes below.

That said, since this PR doesn't seem to actually do anything with / allow usage of is404 and since connectedNode is an unnecessary perf hit. I'm going to put this on hold.

If exposing template date or fixing the error codes gets unblocked before snapwp-helper can expose a templateByUri.is404, then we'll merge this as a temporary fix. Otherwise, we'll update this to use that eventual field.

[justlevine]

Suggested change

	const is404 = templateByUri?.connectedNode === null;
	// @todo templateByUri should expose a `is404` field to avoid the perf hit from connectedNode.
	const is404 = templateByUri?.connectedNode === null;

[justlevine]

@ayushnirwal taking into consideration future-compat with other protentional route-based conditionals: is this the ideal schema shape for consumption?

[Swanand01]

@ayushnirwal This PR for now just returns the is404 property from the parseQueryResult. However, it is still not accessible in the user land. Any ideas on how that can be achieved?

[Swanand01]

In TemplateRenderer:

const { editorBlocks, bodyClasses, stylesheets, scripts, scriptModules, is404 } =
		await getTemplateData( pathname || '/' );
<main>
    <div className="wp-site-blocks">
        { children( { editorBlocks, is404 } ) }
    </div>
</main>

export default function Page() {
	return (
		<TemplateRenderer>
			{ ( { editorBlocks, is404 } ) => {
				return <EditorBlocksRenderer editorBlocks={ editorBlocks } />;
			} }
		</TemplateRenderer>
	);
}

We can expose the is404 to user land this way.

[ayushnirwal]

We can redirect to next's 404 page if is404 is true. If next provides ways to attach status code we can do that.

[justlevine]

We can redirect to next's 404 page if is404 is true. If next provides ways to attach status code we can do that.

The acceptance criteria for https://github.com/rtCamp/headless/issues/451 is to use WordPress as the source of truth for the 404 template while providing user control of status codes based on WPGraphQL-exposed data. I don't think a component-level redirect is an "ideal" DX solve for this, but we can refine it in a follow-up PR.

[Copilot]

Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.

Files not reviewed (1)

• packages/query/src/queries/get-current-template.graphql: Language not supported

Comments suppressed due to low confidence (1)

packages/query/src/utils/tests/parse-template.test.ts:90

• There is no test case for when 'templateByUri' is undefined; consider adding one to ensure the 'is404' flag defaults to false in that scenario.

});

[Swanand01]

We can redirect to next's 404 page if is404 is true.

Not a big fan of doing this, because the user won't be able to control what is to be done if the page is a 404 page.
With my current implementation, they can at least write their own 404 handling logic in page.tsx.

[Swanand01]

@ayushnirwal @justlevine I have updated the current path middleware to perform the is404 check.
The obvious caveat here is that QueryEngine.getTemplateData is called twice for every request, which is why I think that we should have a separate, performant function to check the 404 status for a path.

https://github.com/Swanand01/snapwp/blob/8202409e654cbbb7c394b5c371bfeb91ba4298db/packages/next/src/middleware/current-path.ts#L18-L62

[justlevine]

Why do we need a custom header? Why would we want a custom header?

[Swanand01]

This was added for testing purposes. Removing it now.

[justlevine]

If we can get this out of getTemplateData() we do we also need to expose it directly?

[Swanand01]

I am not entirely sure what you mean here. I assume you mean one of these:

Why is 404 status added to the response when we can get is404 out of getTemplateData()?

• Acceptance criteria. We can get rid of this flag in template renderer if @ayushnirwal says so.

Why do we need a separate function to get 404 status?

• getTemplateData processes a big GetCurrentTemplate query, which has other fields. All we need for now is the connectedNode, so it makes sense to have it separate.

[justlevine]

Sorry, it was your first guess: I was asking about the line I commented on: the is404 param that we're passing to the children.

Can you point me to where that acceptance criteria is listed? Afaik I'm the one who specced both #150 and https://gihtub.com/rtCamp/headless/issues/451 not @ayushnirwal (though obviously if Ayush - or you - has a compelling case for why we need it, I'm happy to defer).

[Swanand01]

From #145 (comment):

This is meant to be a quick fix to add a status code to the response. The changes to the parser may be moved to Template rendering. After #123

From #151 (comment):

If next provides ways to attach status code we can do that.

This wasn't acceptance criteria, but what Ayush suggested. I think it makes sense to have the status code as 404, instead of letting it be 200, and passing is404: true to the user.

[justlevine]

The obvious caveat here is that QueryEngine.getTemplateData is called twice for every request,

The bigger caveat is that you shouldn't be querying data in middleware altogether... If you can avoid it.

Not entirely sure the purpose of all this logic, but my assumption is that the status handling needs to happen in a route handler, not middleware. @ayushnirwal ?

[Swanand01]

The obvious caveat here is that QueryEngine.getTemplateData is called twice for every request,

The bigger caveat is that you shouldn't be querying data in middleware altogether... If you can avoid it.

Not entirely sure the purpose of all this logic, but my assumption is that the status handling needs to happen in a route handler, not middleware. @ayushnirwal ?

We cannot render JSX (<TemplateRenderer>) in a route handler, and there cannot be a route.js file at the same route as page.js. Ref. I'm not sure what can be done here. @ayushnirwal WDYT?

[ayushnirwal]

@Swanand01 I don't know how you arrived at the middleware solution.

Try using the notFound function along with not-found.js file.

1. Create a not-found file that fetches template data for WP 404 page. Since this fetch does not rely on any dynamic parameters, it will be statically built.

2. In Template Renderer, call the function notFound.

@justlevine Is there a reliable URI to get the 404 template in (1) through templateByURI? or any other way?

If no, we can use a garbage URI to fetch 404 template data for now and later update the backend to remove the hack.

[Swanand01]

@SH4LIN I don't know how you arrived at the middleware solution.

Try using the notFound function along with not-found.js file.

1. Create a not-found file that fetches template data for WP 404 page. Since this fetch does not rely on any dynamic parameters, it will be statically built.
2. In Template Renderer, call the function notFound.

@justlevine Is there a reliable URI to get the 404 template in (1) through templateByURI? or any other way?

If no, we can use a garbage URI to fetch 404 template data for now and later update the backend to remove the hack.

What if we use TemplateRenderer in not-found.tsx?

export default function NotFound() {
	return (
		<TemplateRenderer>
			{ ( { editorBlocks } ) => {
				return <EditorBlocksRenderer editorBlocks={ editorBlocks } />;
			} }
		</TemplateRenderer>
	);
}

[ayushnirwal]

What if we use TemplateRenderer in not-found.tsx?

Yes...

[justlevine]

Is there a reliable URI to get the 404 template in (1) through templateByURI? or any other way?

There's currently no way to fetch just a template and not a "resolved" URI. Ideally it would be the same results of the previous query - either via the app or because we have server-side caching. I know most things are locked down, but even just requerying the same URI will give us what we need.

If no, we can use a garbage URI to fetch 404 template data for now and later update the backend to remove the hack.

I'm not opposed to this as a temporary workaround.