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.normalize(undefined) throws on Mac but not on Windows #1139

Closed
bpasero opened this issue Mar 13, 2015 · 11 comments
Closed

Path.normalize(undefined) throws on Mac but not on Windows #1139

bpasero opened this issue Mar 13, 2015 · 11 comments
Labels
path Issues and PRs related to the path subsystem. windows Issues and PRs related to the Windows platform.

Comments

@bpasero
Copy link
Contributor

bpasero commented Mar 13, 2015

Passing in "undefined" into Path.normalize() throws on Mac but works just fine on Windows. I think the issue is that Path.isAbsolute() does not check if path is null or undefined in https://github.com/iojs/io.js/blob/v1.x/lib/path.js#L440

@brendanashworth brendanashworth added path Issues and PRs related to the path subsystem. windows Issues and PRs related to the Windows platform. labels Mar 13, 2015
@brendanashworth
Copy link
Contributor

Can't confirm as I don't run windows (OSX does throw), but I'd certainly like to see some input verification here.

If you're interested, pull requests are welcome 😃

@silverwind
Copy link
Contributor

Confirmed.

Windows:

$ iojs -p "require('path').normalize()"
undefined

OSX and Linux:

$ iojs -p "require('path').normalize()"
path.js:440
  return path.charAt(0) === '/';
             ^
TypeError: Cannot read property 'charAt' of undefined

@silverwind
Copy link
Contributor

What's the expected result? undefined or a custom error perhaps?

@mscdex
Copy link
Contributor

mscdex commented Mar 13, 2015

Or empty string?

@Fishrock123
Copy link
Contributor

path.relative() throws TypeError: Arguments to path.resolve must be strings

(Most of the rest return '.')

@silverwind
Copy link
Contributor

Probably best to throw TypeError in both posix.normalize and win32.normalize. The fact that normalize() returns undefined in win32 makes me a bit uneasy, but I don't think anyone would rely on that functionality, at least not knowingly, so I guess that would count as an bugfix, rather than an API change.

@Fishrock123
Copy link
Contributor

Note: I did not notice earlier, but path.relative() only errors because path.resolve() does.

@piscisaureus
Copy link
Contributor

I don't have an opinion on which behavior is better, but I'd certainly accept a patch that makes behavior consistent across platforms.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 13, 2015

Throwing a TypeError if the input is not a string, sounds like the best bet.

@mscdex
Copy link
Contributor

mscdex commented Mar 13, 2015

+1 for TypeError

cjihrig added a commit that referenced this issue Mar 16, 2015
This commit adds type checking of path inputs to exported methods
in the path module. The exception is _makeLong(), which seems to
explicitly support any data type.

Fixes: #1139
PR-URL: #1153
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
@cjihrig
Copy link
Contributor

cjihrig commented Mar 16, 2015

Fixed in eb995d6

@cjihrig cjihrig closed this as completed Mar 16, 2015
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. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

7 participants