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

Added Windows paths support when requiring local files #61

Merged
merged 2 commits into from
Aug 21, 2023

Conversation

ganigeorgiev
Copy link
Contributor

@ganigeorgiev ganigeorgiev commented Aug 19, 2023

This PR adds a OS dependent absolute path check when requiring local files in order to support Windows absolute paths (eg. C:\...).

Couple notes:

  • I'm not sure actually if this should be allowed because the node file modules docs doesn't mention explicitly whether Windows abs path are resolvable or not (or at least I couldn't find).
    I've tested with Windows 10 VM and Node 18 and require("C:\\...") works there.

  • I'm not sure whether the OS specific separators for ./ and ../ should also be added.

Edit: for more context, this was reported in pocketbase/pocketbase#3163.

@dop251
Copy link
Owner

dop251 commented Aug 20, 2023

The way the documentation is written specifically says it shouldn't work. From the portability point of view it would make sense to claim that the argument to require() is a module path, not a file path and so should always contain forward slashes regardless of the OS (as stated here). Unfortunately this does not work well with absolute Windows paths, so I guess the issue above was 'fixed' at some point, but it was not reflected on the documentation.

I don't mind using filepath.IsAbs(), as for the \. and ..\, I think they should be added, but only for Windows.

@ganigeorgiev
Copy link
Contributor Author

I've added checks for .\ and ..\ and updated the test.

@ganigeorgiev ganigeorgiev changed the title Added OS dependent absolute path check when requiring local files Added Windows paths support when requiring local files Aug 20, 2023
@dop251 dop251 merged commit 94e5081 into dop251:master Aug 21, 2023
8 checks passed
@ganigeorgiev ganigeorgiev deleted the windows-abs-path branch August 21, 2023 14:03
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