Skip to content

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