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 an issue where on POSIX-like filesystems first '/' in absolute file path would be lost. #45

Merged
merged 1 commit into from
Apr 29, 2018

Conversation

gordeevbr
Copy link
Contributor

Greetings!

I have stumbled across the same issue as #42 and I think I have found the cause.

Using path.join('./', context, importThisFile) in the file reading function for #import directive of mock files just so happens to be removing the first '/' from the absolute file path on POSIX-like file systems. So instead of /user/something/whatever/file.json one would get user/something/whatever/file.json which would cause an error.

Since this issue seems to be relatively not very popular I guess it only happens under some circumstances. It appears to be that using just path.join(context, importThisFile) will solve this problem. I'm not sure why path.join('./', context, importThisFile) was used before since "context" variable is assigned path.parse(file).dir + '/' in the code and NodeJS docks say it will evaluate to an absolute directory path on both file systems anyway. Please correct me if I'm wrong here.

Please review this and merge if necessary.

@gordeevbr
Copy link
Contributor Author

P.S. all tests still pass.

@odino odino merged commit 5ce45d1 into namshi:master Apr 29, 2018
@odino
Copy link
Contributor

odino commented Apr 29, 2018

Thanks!

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.

2 participants