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

Used Path.Join to concantenate folder location strings. This fix will fix issues using the project in windows #209

Merged
merged 1 commit into from
Apr 17, 2015

Conversation

michaelpuzon
Copy link
Contributor

...x issues with using the project in windows

… fix issues with using the project in windows
markstos added a commit that referenced this pull request Apr 17, 2015
Used Path.Join to concantenate folder location strings. This fix will fix issues using the project in windows
@markstos markstos merged commit 567c759 into node-config:master Apr 17, 2015
@lorenwest
Copy link
Collaborator

The underlying require('fs') code does windows slash translation for us, so this won't result in any behavior change.

On one hand, this suggests that we've thought about windows slashes. On the other hand, it shows that we think it matters, when it doesn't.

tomato tamato

@michaelpuzon
Copy link
Contributor Author

Hi guys, I encountered this problem because I am developing a node app in windows setting. It was not loading the config folder at all, and the hard-coded slash on the string concantenation was the culprit.

@markstos
Copy link
Collaborator

If this fixes things for a Windows user and doesn't break things for other users, I'm fine with the patch. I know the pattern is common in other languages.. at least Perl uses the same kind of pattern to deal with slash portability.

@lorenwest
Copy link
Collaborator

Now I'm confused. Node-config has been working under windows for years, and your experience differs from my understanding of the fs package.

I wonder if the fs package really doesn't do slash translation. Anyone with a windows machine that would be able to try it out?

var fs=require('fs');
var content = fs.readFileSync('/somedir/somefile.txt');

I think that will work on windows as long as there's a \somedir\somefile.txt file out there. It's more for general understanding of this issue than for this pull request. I'm on board with this pull request.

-Loren

@markstos
Copy link
Collaborator

The path-translation feature of fs sounded familiar to me, but I don't see it documented in the official docs:

https://nodejs.org/api/fs.html

What version of Node.js on Windows did the fs slash translation fail on @michaelpuzon ?

@michaelpuzon
Copy link
Contributor Author

Hi. I am on v0.12.0 for node.js running on 64-bit windows 7.

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.

3 participants