Dest symlinks

[phated]

@erikkemperman does this address your thoughts from your last comment in #239? vfs.symlink doesn't add the .symlink property yet (probably a separate thing to address).

A side thought: the relative option should probably renamed to relativeSymlinks for the vfs.dest method.

[erikkemperman]

Added a few questions and comments -- but mostly this is pretty much what I was trying to do.

[erikkemperman]

Maybe the default should just be a boolean value, set to isWindows? We have to AND the resolved option with isDirectory(file) per file anyway, because we probably shouldn't assume that the useJunctions option provided by the user always satisfies that restriction. Does that make sense?

[phated]

Yeah, this should be a separate PR though, as it's not as simple as changing the default

[erikkemperman]

All right, that makes sense.

[erikkemperman]

Actually, isn't it as simple as changing the default? Seems to me that the non-directory case will be forced to symtype 'file' anyway, and the call to file.isDirectory() will have to be done in write-contents/write-symbolic-link in any event, so no need to do it twice?

Sorry if I'm missing something obvious, it's late here and been one of those days :-)

[phated]

We have to introduce isDirectory into the relative branch, I think.

[erikkemperman]

I wasn't sure here if the criterion should be existence of file.symlink and/or file.isSymbolic(). Ideally, if vinyl-fs is consistent with adding the symlink property, it should not matter I guess. But as things stand, the file.symlink test only catches the vinyls as produced by src() with falsy resolveSymlinks option (for this file), I believe.

[phated]

I'm nervous about using the isSymbolic() method within vinyl-fs methods because I tried to use it in some other code I was experimenting with and it wasn't acting as I thought it would. I found out this was due to not having a valid stat before I used it but I think checking this property is a better way to handle it because isSymbolic() can be true without having this property (I think?)

[erikkemperman]

Yes, I believe file.isSymbolic() returns true if and only if file.stats && file.stats.isSymbolicLink(). So that doesn't work for stat-less files.

Relying on symlink here should be fine, I think, but like I mentioned earlier I think that property should be attached in more places than is currently being done.

I had that figured out already though so I could quickly make a PR for that once this one is merged?

[phated]

Yeah, that sounds great. I should be merging this tomorrow.

[erikkemperman]

It seems to me that the vinyls ending up here currently (excepting the mocked ones you added) would be piped in from src() with the resolveSymlinks set to false. But those vinyls will have the stats of the link, and thus file.isDirectory here is not actually testing whether the target is a directory but the link (and of course isDirectory() will always return false for stats that are isSymbolic()).

Actually I think this is a problem in the implementation of symlink() as well: we can't be sure the vinyl has the stats of the target at the moment we need to know if the target is a directory. In fact we just do new fs.Stats() if the vinyl doesn't have stats, in symlink/prepare...

I suppose we should do an extra fs.stat on the target path, here and in symlink -- but only if we really need to know if it's a directory (i.e. on Windows and with the useJunctions option set to true)?

[phated]

People can inject whatever they want in the middle of the pipeline, so they could do anything like what I've mocked (they can even use a different Vinyl source). We can't couple dest to src in that way and are better with a more lax, yet slightly leaky abstraction (with the .symlink property and usage of fs.Stats).

That's a bit rambling; let me know if I should further clarify.

[erikkemperman]

Sorry, my remark wasn't very clear :-)

I agree about the loose coupling, but my point here is that I think the isDirectory call here is wrong in the common case of getting a symbolic vinyl as produced by src() with resolveSymlinks set to false!

Because it is looking at the wrong stats in that case, namely those of the link and not the target, and then isDirectory will return false even though the target might be a directory, and as a consequence the symlink type should've been'junction' but now won't be.

Does that help?

If you change the test for this, or add a new one, to work with an actually sourced symlink rather than one with a mock isDirectory()

[erikkemperman]

... I think you will see what I mean. (Sorry, hit the button prematurely)

[phated]

(Downloading the suggested patch so sending this now)

@erikkemperman The following example shows something a user might do that would require us to handle this case:

vfs.src('./my-giant-image-dir')
  .pipe(through(function toSymlink(file, _, cb) {
    // Since a directory was sourced, file.isDirectory() will be true and we'll want to create a junction on Windows
    file.symlink = file.path;
  })
  .pipe(vfs.dest('./my-output-location'))

[phated]

I'll give that a shot.

[phated]

Proved that Windows does not handle things properly if you force 'junction' at https://ci.appveyor.com/project/gulpjs/vinyl-fs/build/487

[phated]

I don't think using another fs.stat is proper because the Vinyl object doesn't need to be produced from the filesystem, so that call would only be correct in some circumstances (yet still incorrect in others).

[erikkemperman]

True, but it would treat correctly it least the cases where it has the necessary information to, or so it seems to me.

It's too bad that node doesn't enforce this for us, but good to have that confirmed at least.

Having read up a little bit on junctions I'm wondering if the current approach, where it'd be the default even, may end up causing more problems than it solves?

As far as I can tell the problem was that symlinking dirs (but not files?) requires admin privileges.

But junctions are dangerous in that they may cause users to lose data (c.f. the caveats in the comments).

Then again that's easy for me to say because I don't use Windows.

[phated]

npm exclusively uses junctions on Windows, so I think the caveats were over-cautious.

[erikkemperman]

Maybe this ought to be called target, as in the fs.symlink call? Personally I think source is a bit confusing in this context, I think of this as the destination of the link :-)

[phated]

From a semantic standpoint, you are correct. But naming it target inverts my expectation (as someone that doesn't link very often). I'm fine with altering naming conventions (with proper documentation comments) but I don't want to hold up release of 3.0

[erikkemperman]

Fair enough, although I'm not sure what you mean by documenting it?

The variable has no external visibility and is only used briefly within the innermost scopes of dest/write-contents/write-symbolic-link and symlink/index?

[phated]

Just a code comment explaining what "target" means in the context.

[erikkemperman]

Would it be possible to deduplicate the junction voodoo and do it within fo.symlink, now that we have that separate function which is called from both dest() and symlink()?

[phated]

I thought about that, but I'd rather ship this last feature to get 3.0 released and work on de-duplication in a future release.

[erikkemperman]

All right, yeah it shouldn't be a breaking change.

[stephenlacy]

@phated I cloned to the branch and ran the tests, several failed for me on Linux. All were to do with the filesystem returning errors. I can provide detailed information as required.

[phated]

@stevelacy These all pass on travis, so I am wondering what is different on your system. Not sure if it belongs in this PR and might be something we look at in person

[erikkemperman]

@stevelacy Were your errors resolved now? I ask because, after nuking node_modules and doing a fresh yarn install I suddenly also get errors on previously working code. And both Travis and Appveyor fail now, even on master:

• https://ci.appveyor.com/project/erikkemperman/vinyl-fs/build/80
• https://travis-ci.org/erikkemperman/vinyl-fs/builds/245354284

Will take a quick look to see if I can figure out what is going on.

EDIT The common theme of the failing tests is that errors are no longer being propagated correctly, but unfortunately I have no time right now to look into this further.

[stephenlacy]

@erikkemperman @phated yes, those are exactly my errors. I could take a look at them once I get some free time

[phated]

Seeing these on OSX too when deps updated. Investigating

[phated]

This looks like a problem with readable-stream because it seems to be the only dep that updated

[phated]

@stevelacy @erikkemperman this is definitely a problem introduced in readable-stream as backported from node 8. I'm working with them to get it fixed.

[erikkemperman]

@phated Thanks for the update.

FYI I'm trying to get an extra test done this evening if I find some time, that might clarify the problem I believe still remains with isDirectory() in this PR.

[phated]

@erikkemperman I'm pretty clear on what you are saying but you are thinking of it from an integration point-of-view. I will post an example of why we can't make those assumptions (after I deal with this nodejs/streams issue).

[erikkemperman]

@phated All right, I'll hang back for a bit :-)

[stephenlacy]

All tests passing on node 8.1.2 now

[phated]

@erikkemperman I believe I've created issues for the 4 outstanding items in this PR. I think we should work on them in smaller chunks so I've merged this and we can work from there.

[erikkemperman]

@phated Yup, thanks!