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

fix(fileserver):wrong url href of displayed files #426

Merged
merged 5 commits into from
May 22, 2019

Conversation

yuqingc
Copy link
Contributor

@yuqingc yuqingc commented May 21, 2019

@CLAassistant
Copy link

CLAassistant commented May 21, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@zekth zekth left a 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

@@ -162,7 +162,7 @@ async function serveDir(
listEntry.push(
createDirEntryDisplay(
info.name,
fn,
fn.replace(currentDir, ''),
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Member

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...

Copy link
Contributor Author

@yuqingc yuqingc May 21, 2019

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.

Copy link
Contributor Author

@yuqingc yuqingc May 21, 2019

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.

Copy link
Contributor Author

@yuqingc yuqingc May 22, 2019

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.

@yuqingc yuqingc changed the title fix(fileserver):wrong url href of displayed files wip:fix(fileserver):wrong url href of displayed files May 21, 2019
@yuqingc
Copy link
Contributor Author

yuqingc commented May 21, 2019

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.

@yuqingc yuqingc changed the title wip:fix(fileserver):wrong url href of displayed files fix(fileserver):wrong url href of displayed files May 22, 2019
@yuqingc
Copy link
Contributor Author

yuqingc commented May 22, 2019

Done. Deno.FileInfo is not compatible with Windows yet, which would raise an error during testing checking. Thus I added an extra checking of the platform.

@piscisaureus
Copy link
Member

LGTM.
File mode is not a thing on windows any any attempts to emulate it are useless.

@piscisaureus piscisaureus merged commit be6cd35 into denoland:master May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants