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.

Sign up for GitHub

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: 404.html load correctly on preview #9907

Merged
merged 4 commits into from
Feb 2, 2024
+103 −23
Merged

Fix: 404.html load correctly on preview #9907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a7164d
Move vite 404 middleware
ktym4a Jan 31, 2024
c3a0208
Add custom 404.html rendering test for preview routing
ktym4a Feb 2, 2024
5096140
add a changest
ktym4a Feb 2, 2024
940f2c6
add TODO comment
ktym4a Feb 2, 2024
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
5 changes: 5 additions & 0 deletions .changeset/tidy-deers-double.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"astro": patch
---

Load 404.html on all non-existent paths on astro preview.
39 changes: 20 additions & 19 deletions packages/astro/src/core/preview/vite-plugin-astro-preview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,18 @@ const HAS_FILE_EXTENSION_REGEXP = /^.*\.[^\\]+$/;
export function vitePluginAstroPreview(settings: AstroSettings): Plugin {
const { base, outDir, trailingSlash } = settings.config;

function handle404(req: IncomingMessage, res: ServerResponse) {
const errorPagePath = fileURLToPath(outDir + '/404.html');
if (fs.existsSync(errorPagePath)) {
res.statusCode = 404;
res.setHeader('Content-Type', 'text/html;charset=utf-8');
res.end(fs.readFileSync(errorPagePath));
} else {
res.statusCode = 404;
res.end(notFoundTemplate(req.url!, 'Not Found'));
}
}

return {
name: 'astro:preview',
apply: 'serve',
Expand Down Expand Up @@ -48,6 +60,14 @@ export function vitePluginAstroPreview(settings: AstroSettings): Plugin {
}
}

// TODO: look into why the replacement needs to happen here
for (const middleware of server.middlewares.stack) {
// This hardcoded name will not break between Vite versions
if ((middleware.handle as Connect.HandleFunction).name === 'vite404Middleware') {
middleware.handle = handle404;
}
}

next();
Comment on lines +64 to 71
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it necessary to overwrite vite's middleware during the execution of our other middleware?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lilnasy
Thanks, I'll do a little research to see if I can move somewhere properly handle vite404Middleware.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine if you don't find it or if it takes too long. This is already quite an improvement!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add a "// TODO: look into why the replacement needs to happen here" in the meanwhile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added TODO comment.
I will research and send another PR if I think it can be improved.
Thank you very much.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do, we may need to set appType to 'custom' and add HTML middlewares, I think.

References:
https://github.com/vitejs/vite/blob/775bb5026ee1d7e15b75c8829e7f528c1b26c493/packages/vite/src/node/preview.ts#L213-L219
https://github.com/vitejs/vite/blob/main/packages/vite/src/node/server/middlewares/notFound.ts#L3
https://vitejs.dev/config/shared-options.html#apptype

});

Expand Down Expand Up @@ -77,25 +97,6 @@ export function vitePluginAstroPreview(settings: AstroSettings): Plugin {

next();
});

// Vite has its own 404 middleware, we replace it with ours instead.
for (const middleware of server.middlewares.stack) {
// This hardcoded name will not break between Vite versions
if ((middleware.handle as Connect.HandleFunction).name === 'vite404Middleware') {
// Fallback to 404 page if it exists
middleware.handle = (req: IncomingMessage, res: ServerResponse) => {
const errorPagePath = fileURLToPath(outDir + '/404.html');
if (fs.existsSync(errorPagePath)) {
res.statusCode = 404;
res.setHeader('Content-Type', 'text/html;charset=utf-8');
res.end(fs.readFileSync(errorPagePath));
} else {
res.statusCode = 404;
res.end(notFoundTemplate(req.url!, 'Not Found'));
}
};
}
}
};
},
};
Expand Down
82 changes: 78 additions & 4 deletions packages/astro/test/preview-routing.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { expect } from 'chai';
import * as cheerio from 'cheerio';
import { loadFixture } from './test-utils.js';

describe('Preview Routing', function () {
Expand Down Expand Up @@ -182,6 +183,41 @@ describe('Preview Routing', function () {
expect(response.status).to.equal(404);
});
});

describe('Load custom 404.html', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
/** @type {import('./test-utils').PreviewServer} */
let previewServer;

let $;

before(async () => {
fixture = await loadFixture({
root: './fixtures/custom-404-html/',
server: {
port: 4003,
},
});
await fixture.build();
previewServer = await fixture.preview();
});

after(async () => {
await previewServer.stop();
});

it('renders custom 404 for /a', async () => {
const res = await fixture.fetch('/a');
expect(res.status).to.equal(404);

const html = await res.text();
$ = cheerio.load(html);

expect($('h1').text()).to.equal('Page not found');
expect($('p').text()).to.equal('This 404 is a static HTML file.');
});
});
});

describe('build format: file', () => {
Expand All @@ -201,7 +237,7 @@ describe('Preview Routing', function () {
},
trailingSlash: 'never',
server: {
port: 4003,
port: 4004,
},
});
await fixture.build();
Expand Down Expand Up @@ -261,7 +297,7 @@ describe('Preview Routing', function () {
},
trailingSlash: 'always',
server: {
port: 4004,
port: 4005,
},
});
await fixture.build();
Expand Down Expand Up @@ -324,7 +360,7 @@ describe('Preview Routing', function () {
},
trailingSlash: 'ignore',
server: {
port: 4005,
port: 4006,
},
});
await fixture.build();
Expand Down Expand Up @@ -387,7 +423,7 @@ describe('Preview Routing', function () {
},
trailingSlash: 'ignore',
server: {
port: 4006,
port: 4007,
},
});
await fixture.build();
Expand Down Expand Up @@ -423,5 +459,43 @@ describe('Preview Routing', function () {
expect(response.status).to.equal(404);
});
});

describe('Load custom 404.html', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
/** @type {import('./test-utils').PreviewServer} */
let previewServer;

let $;

before(async () => {
fixture = await loadFixture({
root: './fixtures/custom-404-html/',
build: {
format: 'file',
},
server: {
port: 4008,
},
});
await fixture.build();
previewServer = await fixture.preview();
});

after(async () => {
await previewServer.stop();
});

it('renders custom 404 for /a', async () => {
const res = await fixture.fetch('/a');
expect(res.status).to.equal(404);

const html = await res.text();
$ = cheerio.load(html);

expect($('h1').text()).to.equal('Page not found');
expect($('p').text()).to.equal('This 404 is a static HTML file.');
});
});
});
});
Loading