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

case sensitive file handling #177

Closed
wants to merge 2 commits into from

Conversation

sebbi08
Copy link

@sebbi08 sebbi08 commented Jan 4, 2019

This is ment to solve this issue expressjs/serve-static#93.
It contains a fix to handel files on in case insensitive file system case sensitive.

@sebbi08
Copy link
Author

sebbi08 commented Jan 4, 2019

is there any diference betwene the tracis ci coverage reporter and the local one localy i get the following coverage results :

Statements : 99.77% ( 425/426 )
Branches : 96.36% ( 238/247 )
Functions : 100% ( 49/49 )
Lines : 100% ( 416/416 )

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

I just had a couple of things on this one. I apologize for letting this sit with no comments. I hope you are welcome to picking it back up; if you're not, let me know and I can try to champion it forward.

if (dir === path.dirname(dir)) {
return true
}
var filenames = fs.readdirSync(dir)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should really do this using an async method; sync methods will block the event loop and processing of all I/O in the process while the read occurs, and it could be against a network share, for example.

##### caseSensitive

When set to `true` all the requests against the file system are made case sensitive even on case insensitive file systems.
This effects not only the file name but the whole path.
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 just a comment) I assume by "whole path" that includes what was provided also as the root option? If so, it should probably call that out, as there is the input named path, which may be mistake for "the whole path" in this wording.


When set to `true` all the requests against the file system are made case sensitive even on case insensitive file systems.
This effects not only the file name but the whole path.
When set to `false` the default behavior of the filesystem is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Passing in caseSensitive: false does have a weird feel to the API... it feels like it should thus not be case-sensitive, vs depend on the file system. I'm trying to think of a good API name for this functionality, and don't have a good one.

Would it be possible to maybe even make caseSensitive: false work? For example, to have this option be true, or false, or 'fs'? If not, as I cannot think of a good API name that would make sense as true/false, maybe use a string like 'fs'instead of acceptingfalse`?

@@ -546,6 +548,7 @@ SendStream.prototype.pipe = function pipe (res) {

// join / normalize from optional root dir
path = normalize(join(root, path))
root = normalize(root + sep)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this doing, exactly?

@sebbi08 sebbi08 closed this Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants