normalize paths

[phated]

In testing on Windows, I found that the slashes in the path history were incorrect. It seems these are coming from node-glob but we aren't doing any translation.

Not sure where we should do this, but in the meantime, I am normalizing in vinyl-fs before we construct the Vinyl object because it requires the least releases.

cc @contra

[yocontra]

This should happen in vinyl

[phated]

Thanks, should it only happen on construction? I was also thinking that glob-stream should maybe do it so there aren't incorrect paths being streamed on Windows (glob stream emits plain objects, not Vinyl objects)

[Marak]

@contra -

How would this work reliably in vinyl core? To my understanding the only way to find the correct file path on windows is to use fs module? I'm not entirely sure.

Would normalization of paths apply to all vinyl files, or just those created from vinyl-fs?

Is there code kicking around to normalize the paths anywhere? I found https://github.com/gulpjs/vinyl-fs/blob/master/lib/src/wrapWithVinylFile.js#L9, not sure if that is the current solution?

[phated]

@Marak that's currently where we do the normalization (in glob-stream - https://github.com/gulpjs/glob-stream/blob/master/index.js#L50) but my thinking is that we should use path.normalize when a path is set in vinyl instead of deferring that to glob-stream/vinyl-fs

[phated]

Correction: that got moved into glob-stream at https://github.com/gulpjs/glob-stream/blob/master/index.js#L50

[Marak]

I haven't done much windows testing, path.normalize will work?

If so, seems like it would be a simple fix to vinyl?

[phated]

@Marak yeah, node's path.normalize is platform dependent. Should be a simple fix but no one has gotten around to changing + writing tests.

[Marak]

Cool.

I'm putting vinyl into the hook.io dependency tree for some new platform features.

Been doing a general review of the ecosystem and core libs. Very impressed with everything. Thank you.

[darsain]

Replying to #92 (comment) here since this is the place for it :)

I'll probably be very nitpick-y about the normalization stuff because we've had a few problems recently - see: https://github.com/gulpjs/vinyl-fs/blob/master/lib/prepare-write.js#L47-L49 which needs to be moved into vinyl and most of the path stuff needs to be normalized (e.g. directory returning properties should always end in a path.sep, if file.isDirectory() the file.path should also end in path.sep and other things like that)

Why should directory paths end in separator?

[phated]

@darsain since we already use path.* methods in our getters/setters for the path properties, a user shouldn't be required to use path.join to rebuild a path.

Some examples:

// This doesn't currently work
var file = new Vinyl({ path: '/test/123.jpg' });
file.dirname + file.basename === file.path
// Instead you must use path.join which adds another function call
path.join(file.dirname, file.basename) === file.path

// This also doesn't work
file.dirname + file.stem + file.extname === file.path
// So you have to use path.join AND string concatenation
path.join(file.dirname, file.stem + file.extname) === file.path

These inconsistencies also resulted in unexpected results when I was refactoring the vinyl-fs tests. We need to make the behavior consistent and easy to work with.

[phated]

@darsain any progress on this? If not, I might be tackling it soon.

[darsain]

@phated other things creeped in :/ should get back to it sometime in the next few days.

[darsain]

Should we throw in file.path getter when file.history is empty, i.e. there is no path to be retrieved?

Otherwise, accessing stuff like file.dirname with empty history will throw a non-descriptive path.dirname() error Path must be a string. Received undefined, forcing the user to walk the stack trace to figure out what the actual issue is.

The solutions are:

1. Throw Error('object has no path') when retrieving nonexistent file.path.
2. Handle file.path === undefined case in all affected getters and:
   1. return undefined or empty string.
   2. throw Error('object has no path').

Is file.path returning undefined something heavily relied on right now?

In tests, I see that file.dirname() is supposed to throw when there is no path. file.path is supposed to throw only when attempt to set an undefined|null path is made, but there is no documentation or test for retrieving a nonexistent path, so this behavior is currently undefined.

[phated]

@darsain how would you actually set the path to empty? Is there a code path that allows it?

[darsain]

@phated sorry, Going thought the code again made me realize that it already answers these questions, as well as handles the no path case in a solid way :)

But to answer your question, file.path setter does allow empty strings.

[darsain]

Finishing up a PR, have hopefully 2 last questions:

1. Should file.basename end with separator when file.isDirectory() is true? I guess not, but not 100% sure.
2. I'm tweaking tons of tests to pass on windows, which they currently don't due to separators. This will result in a lot of noise in tests, which will have to be plastered with path.normalize() calls. It will be a lot easier to just force path.sep = '/' where we are not testing normalization, and revert when done. Any objections?

Edit: Actually, the path module has 2 versions, posix and win32, and the default one can't be flipped just by setting path.sep, so disregard the 2nd question. Tests will have to be normalized after all.

[phated]

@darsain I think file.basename should end with a separator if isDirectory() is true. I don't know of a good example where it would be useful (but also can't think of anywhere it would be bad); however, I think the consistency with the other methods is important. I think this should be true: file.dirname + file.basename === file.path in all scenarios and path should be normalized to end with a separator.

Aside: I'm excited to see this PR.

[darsain]

@phated damn, just submitted the PR without trailing sep for basename :) gonna add it in.