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

path: Throwing error on zero length strings #2105

Closed
wants to merge 1 commit into from

Conversation

thefourtheye
Copy link
Contributor

The path module's 'join', 'normalize', 'isAbsolute', 'relative'
functions return/use the current directory or ignore the path,
if they are passed zero length strings.

> process.version
'v2.3.4-pre'
> path.join('')
'.'
> path.win32.join('')
'.'
> path.posix.join('')
'.'
> path.win32.normalize('')
'.'
> path.posix.normalize('')
'.'
> path.win32.isAbsolute('')
false
> path.posix.isAbsolute('')
false
> path.win32.relative('', '')
''
> path.posix.relative('', '')
''
> path.win32relative('.', '')
''
> path.posix.relative('.', '')
''

But that is unintuitive. This PR throws an error if any of the
parameters to those functions are zero length strings.

@chrisdickinson
Copy link
Contributor

Changing this behavior will adversely affect a lot of downstream modules. This change would have to be preceded by a deprecation warning for the duration of at least one major version, and it's probable that we wouldn't be able to actually change the behavior until a large number of modules updated first (no matter how many major versions that takes.)

@brendanashworth brendanashworth added path Issues and PRs related to the path subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jul 5, 2015
@domenic
Copy link
Contributor

domenic commented Jul 5, 2015

I don't think the upsides of this (if there are any) are worth the breakage. -1.

The path module's `'join', 'normalize', 'isAbsolute', 'relative'`
functions return/use the current directory or ignore the path,
if they are passed zero length strings.

    > process.version
    'v2.3.4-pre'
    > path.join('')
    '.'
    > path.win32.join('')
    '.'
    > path.posix.join('')
    '.'
    > path.win32.normalize('')
    '.'
    > path.posix.normalize('')
    '.'
    > path.win32.isAbsolute('')
    false
    > path.posix.isAbsolute('')
    false
    > path.win32.relative('', '')
    ''
    > path.posix.relative('', '')
    ''
    > path.win32relative('.', '')
    ''
    > path.posix.relative('.', '')
    ''

But that is unintuitive. This PR throws an error if any of the
parameters to those functions are zero length strings.
@thefourtheye
Copy link
Contributor Author

No problem :-) Yesterday, when I was trying to understand the option mentioned in this comment, I simply used '' in fs.watchFile, expecting it to invoke the callback function, but it didn't. When I actually give an invalid path, it invoked the callback function. So, I went to the source code to understand why this happens. At this point, I didn't even imagine path.normalize call here would return the current directory. I checked the C++ part and couldn't find anything there and went into the uv source code to understand what it does and there also nothing interesting was done. So I introduced log statements in the C++ code and printed the error returned by uv_fs_poll_start here (Note: I am not sure why we are ignoring the error returned by uv_fs_poll_start). It was also printing zero. So, I started reading the fs.watchFile again, line by line and this time I figured that resolve returns the current directory, if an empty string is passed. I spent considerable amount of time figuring this out and I thought it would be not easy to debug for others also. Anyway, an empty string is not a valid filename. So, I thought issuing an error would be the best action here.

As the breakage is not justified by this PR, I am closing this and opening another PR #2106, targeting master, with the documentation changes. I hope that is okay.

@thefourtheye thefourtheye deleted the empty-path-fix branch July 5, 2015 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
path Issues and PRs related to the path subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants