Ensure local dependencies are properly copied on Windows

[oliversalzburg]

Platforms affected

All

What does this PR do?

Ensures that dependencies are properly copied when the source is a symlink/junction.

What testing has been done on this change?

E2E test suite has been run and now finally passes on Windows.

Checklist

• Reported an issue in the JIRA database
• Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
• Added automated test coverage as appropriate for this change.

[janpio]

Makes the tests pass for me 🥇

[knight9999]

It looks nice!
As this PR, I feel that copy is better than link.
because in some case the directory owner is different user.

[erisu]

Looks good to me.

[raphinesse]

Wow, this seems to fix quite a few obscure problems. Nice work on the debugging everyone! 👍

Could you explain what exactly went wrong with the previous version of the code and how adding the dereference option fixes that?

If the source is a link, will the destination be a link too, or an actual copy of the folder pointed to by the source link?

[janpio]

Could you explain what exactly went wrong with the previous version of the code and how adding the dereference option fixes that?

This is the code dereference influences:
https://github.com/jprichardson/node-fs-extra/blob/402c1d05727f2f9414a4905ffa75f4fa1d99461c/lib/copy-sync/copy-sync.js#L40-L47

Previosuly, this did use fs.lstatSync to get information on the directory, which concluded that it is a symlink, not a directory, and executed onLink which creates another symlink https://github.com/jprichardson/node-fs-extra/blob/402c1d05727f2f9414a4905ffa75f4fa1d99461c/lib/copy-sync/copy-sync.js#L129 Unfortunately this one didn't properly work on Windows because... @oliversalzburg will have to explain that part. (my interpretation: it created a symlink for a file, which is bad if the target is a folder.)

Now, with the option, fs.statSync is used which looks inside the actual symlink and discovers the folder. Then onDir is triggered, which does more useful things.

In normal usage of the method this is possibly not even a problem, but in the test setup the Test1 plugin in Test #29 is first symlinked to a location in node_modules, and then that one is copied/symlinked again into the new project. If the code operates on a real folder, not a symlink, it worked before as it should.

[raphinesse]

Thanks for the details @janpio. What about my last question? Do you know how the new code behaves?

If the source is a link, will the destination be a link too, or an actual copy of the folder pointed to by the source link?

[janpio]

Not really, I would have to check (but on a Chromebook right now). I had a solution that created another symlink (with junction, whatever that is exactly) but it was an ugly hack that circumvented fs.copySync completely and @oliversalzburg 's actually uses the code how it should be used I think.

[oliversalzburg]

To summarize the issue and solution:

When you npm install a local path, npm will create a link from the local path to your target node_modules. When npm creates links, it will always pass "junction" as the third argument. This argument is only relevant on Windows and the default value is "file".

Windows distinguishes between different types of links, just like UNIX does, but differently. The most important distinctions are between file and directory symlinks and junctions (or reparse points). Instinctively, every developer would want to just use symlinks on Windows as well, but junctions are usually preferable. A symlink on Windows is much more versatile, but we usually don't care about the extra functionality and we also don't want to care about the file/directory distinction. For all intents and purposes, a junction is the way to go. As junctions can only be used to link directories and symbolic links are problematic, you are left with the limitation that you can really only easily and properly link directories on Windows.

So when you instruct NodeJS to create a symlink on Windows and you don't pass a third argument, NodeJS will create a file symlink. This will usually fail, because the user is not allowed to create symlinks. That is one of the main reasons to always use junctions on Windows.

Now when we're instructing fs-extra to copy the directory (which is an existing junction), it will detect it as being a symlink (in its own terms; it's still a junction) and instead of copying just create a link between source and destination without specifying "junction" as the third argument. This will instruct NodeJS to create a file symlink. That either fails instantly or creates the file symlink when the target is actually a directory.

So by passing dereference, we're letting fs-extra know that we care about the referenced directory and not the link itself. It will then properly copy the directory.

I'm not sure why fs-extra doesn't use "junction" or any other kind of detection for this when creating symlinks.

[raphinesse]

Alright, thanks for the detailed explanation @oliversalzburg. I was mainly interested in the second part (specific to this issue; was already familiar with the general linking behavior of npm and Windows).

Since we have a dedicated option for when we actually want to create a link, I think this fix is fine. It would be nice to have a test to verify the behavior for when the source is a link. But we can merge this either way.

We might even consider the behavior of fs-extra buggy. Shouldn't it check the source link's type when copying it and create a link of matching type?

[oliversalzburg]

We might even consider the behavior of fs-extra buggy. Shouldn't it check the source link's type when copying it and create a link of matching type?

I think so, yeah. I opened jprichardson/node-fs-extra#626 to discuss this with the authors.

[raphinesse]

I think this is the most-approved PR I've seen here thus far 😁
Congrats, @oliversalzburg 🥇