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

fixed served directories with index.html #22

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

fixed served directories with index.html #22

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ const handleFileRequest = async (req: ServerRequest) => {

const handleRouteRequest = async (req: ServerRequest): Promise<void> => {
try {
const file = await readFile(`${root}/${entryPoint}`)
const file = await readFile(`${root}${req.url}${entryPoint}`)
Copy link
Owner

Choose a reason for hiding this comment

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

I think this will break SPA functionality, no?

If your root is for example /app, and you request /app/some-route/, it will look for /app/some-route/index.html which doesn't exist since it is still in /app/index.html

Copy link
Author

Choose a reason for hiding this comment

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

Mmm, ok I didn't understand the purpose of entryPoint, because by default is index.html. Maybe we need an option to decide the router behaviour. Somethink like spa: false (so it will work as a simple static file server, but resolving paths to directories appending "index.hml" or spa: "/app.html" (so any not found request will resolve to the "app.html" file (or any other that's configured)

Copy link
Owner

Choose a reason for hiding this comment

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

Oh my, I just realized that I had not included the directory listing / static file server documentation in the readme.

Denoliver has something similar denoliver -l which disables routing and resolves paths to directories instead.

Have you tried that out? I will update the readme. Thanks for pointing it out

Copy link
Author

Choose a reason for hiding this comment

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

Ok, that works better but does not fix the whole problem. In my opinion, if a directory contains an index.html file, it shouldn't list the files, but show this file, the same behaviour as any webserver. Can this behaviour be changed? Or provide an option for that?

Copy link
Author

Choose a reason for hiding this comment

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

Another issue I found (not related with this one) is that BrokenPipe error appears randomly:

imaxe

This is due req.respond() is an async function, so it should be prepended with an await, so the error can be catched:

try {
   await req.respond({...});
catch (err) {
    //ignore error
}

Copy link
Owner

@joakimunge joakimunge Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, now I see what you mean. Sorry about that. No, that is definitely not intented behaviour. index files should be served if they exist. Do you want to work on this? It needs to take the list flag into account when determining if it should serve the index.html file or not.

Also a test for this case would be great.

Regarding the other issue, feel free to open a PR. Otherwise I will get to it soon

const { hostname, port } = req.conn.localAddr as Deno.NetAddr
req.respond({
status: 200,
Expand Down