-
Notifications
You must be signed in to change notification settings - Fork 619
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
fix(fileserver):wrong url href of displayed files #426
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/denoland/deno_std/pull/426/files#diff-90919ab1e691a7a991a235f6b575c226R150
change:
const fileInfos = await readDir(dirPath);
by
const fileInfos = await readDir(dirPath);
dirPath = dirPath.replace(currentDir, "");
Also please format your code :) use:
deno run --allow-run --allow-write --allow-read format.ts
http/file_server.ts
Outdated
@@ -162,7 +162,7 @@ async function serveDir( | |||
listEntry.push( | |||
createDirEntryDisplay( | |||
info.name, | |||
fn, | |||
fn.replace(currentDir, ''), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not good. We don't have to replace the dir on each occurence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your comment. I'll fix and test it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try using dirPath + "/" + info.name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to get a test of this failure in http/file_server_test.ts
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try using
dirPath + "/" + info.name
The original version is dirPath + "/" + info.name
. However, we get the absolute OS path of the current directory as dirPath
, which is not correct. We should trim the prefix of currentDir
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zekth We should not modify the variable dirPath
because there are codes using it below ( in line 161, stat(fn)
), where stat
needs the full path of a file as its argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ry I have added two more test cases. All tests passed. Now the test would fail on either condition as you mentioned above.
Please hold this PR and do not merge for now. I have network problems and I have to retest the new commits when my nerwork works again. |
Done. |
LGTM. |
#425