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

feat(spas): streamline 404 #12248

Merged
merged 3 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 2 additions & 2 deletions build/spas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,9 @@ export async function buildSPAs(options: {

// The URL isn't very important as long as it triggers the right route in the <App/>
const locale = DEFAULT_LOCALE;
const url = `/${locale}/404.html`;
const url = `/${locale}/404/index.html`;
const context: HydrationData = { url, pageNotFound: true };
const outPath = path.join(BUILD_OUT_ROOT, locale.toLowerCase(), "_spas");
const outPath = path.join(BUILD_OUT_ROOT, locale.toLowerCase(), "404");
fs.mkdirSync(outPath, { recursive: true });
const jsonFilePath = path.join(
outPath,
Expand Down
2 changes: 1 addition & 1 deletion client/src/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ export function App(appProps: HydrationData) {
to simulate it.
*/}
<Route
path="/_404/*"
path="/404/*"
element={
<StandardLayout>
<PageNotFound />
Expand Down
4 changes: 2 additions & 2 deletions client/src/page-not-found/fallback-link.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,10 @@ export default function FallbackLink({ url }: { url: string }) {
// What if we attempt to see if it would be something there in English?
// We'll use the `index.json` version of the URL
let enUSURL = url.replace(`/${locale}/`, "/en-US/");
// But of the benefit of local development, devs can use `/_404/`
// But of the benefit of local development, devs can use `/404/`
// instead of `/docs/` to simulate getting to the Page not found page.
// So remove that when constructing the English index.json URL.
enUSURL = enUSURL.replace("/_404/", "/docs/");
enUSURL = enUSURL.replace("/en-US/404/", "/en-US/docs/");

// The fallback check URL should not force append index.json so it can
// follow any redirects
Expand Down
2 changes: 1 addition & 1 deletion client/src/page-not-found/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const FallbackLink = React.lazy(() => import("./fallback-link"));

// NOTE! To hack on this component, you have to use a trick to even get to this
// unless you use the Express server on localhost:5042.
// To get here, use http://localhost:3000/en-US/_404/Whatever/you/like
// To get here, use http://localhost:3000/en-US/404/Whatever/you/like
// Now hot-reloading works and you can iterate faster.
// Otherwise, you can use http://localhost:5042/en-US/docs/Whatever/you/like
// (note the :5042 port) and that'll test it a bit more realistically.
Expand Down
2 changes: 1 addition & 1 deletion cloud-function/src/handlers/proxy-content.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { Source, sourceUri } from "../env.js";
import { PROXY_TIMEOUT } from "../constants.js";
import { isLiveSampleURL } from "../utils.js";

const NOT_FOUND_PATH = "en-us/_spas/404.html";
const NOT_FOUND_PATH = "en-us/404/index.html";

let notFoundBuffer: ArrayBuffer;

Expand Down
2 changes: 1 addition & 1 deletion docs/spas.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ To debug the 404 page, in local development you have two choices:

- <http://localhost:5042/en-US/docs/Does/not/exist>

- <http://localhost:3000/en-US/_404/Does/not/exist>
- <http://localhost:3000/en-US/404/Does/not/exist>

The latter is used so you get hot-reloading as you're working on it. This will
only work when you do local development on Yari.
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
"prettier-check": "prettier --check .",
"prettier-format": "prettier --write .",
"render:html": "cross-env NODE_ENV=production NODE_OPTIONS=\"--no-warnings=ExperimentalWarning --loader ts-node/esm\" node build/ssr-cli.ts",
"start": "(test -f client/build/asset-manifest.json || yarn build:client) && (test -f ssr/dist/main.js || yarn build:ssr) && (test -f popularities.json || yarn tool popularities) && (test -d client/build/en-us/_spas || yarn tool spas) && (test -d client/build/en-us/_spas/404.html || yarn render:html -s) && nf -j Procfile.start start",
"start": "(test -f client/build/asset-manifest.json || yarn build:client) && (test -f ssr/dist/main.js || yarn build:ssr) && (test -f popularities.json || yarn tool popularities) && (test -d client/build/en-us/404 || yarn tool spas) && (test -d client/build/en-us/404/index.html || yarn render:html -s) && nf -j Procfile.start start",
"start:client": "cd client && cross-env NODE_ENV=development BABEL_ENV=development PORT=3000 node scripts/start.js",
"start:rari": "(test -f client/build/asset-manifest.json || yarn build:client) && (test -f ssr/dist/main.js || yarn build:ssr) && cross-env RARI=true nf -j Procfile.rari start",
"start:rari-external": "(test -f client/build/asset-manifest.json || yarn build:client) && (test -f ssr/dist/main.js || yarn build:ssr) && cross-env RARI=true nf -j Procfile.start start",
Expand Down
2 changes: 1 addition & 1 deletion server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ async function send404(res: express.Response) {
if (!RARI) {
return res
.status(404)
.sendFile(path.join(STATIC_ROOT, "en-us", "_spas", "404.html"));
.sendFile(path.join(STATIC_ROOT, "en-us", "404", "index.html"));
} else {
try {
const index = await fetch_from_rari("/en-US/404");
Expand Down
2 changes: 1 addition & 1 deletion server/static.ts
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ app.get("/*", async (req, res) => {
console.log(`Don't know how to mock: ${req.path}`, req.query);
res
.status(404)
.sendFile(path.join(BUILD_OUT_ROOT, "en-us", "_spas", "404.html"));
.sendFile(path.join(BUILD_OUT_ROOT, "en-us", "404", "index.html"));
});

const HOST = process.env.SERVER_HOST || undefined;
Expand Down
4 changes: 2 additions & 2 deletions testing/tests/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1173,9 +1173,9 @@ test("images that are in the folder but not in <img> tags", () => {
});

test("404 page", () => {
const builtFolder = path.join(buildRoot, "en-us", "_spas");
const builtFolder = path.join(buildRoot, "en-us");
expect(fs.existsSync(builtFolder)).toBeTruthy();
const htmlFile = path.join(builtFolder, "404.html");
const htmlFile = path.join(builtFolder, "404", "index.html");
const html = fs.readFileSync(htmlFile, "utf-8");
const $ = cheerio.load(html);
expect($("title").text()).toContain("Page not found");
Expand Down
Loading