Add flag to follow symbolic links when crawling files #4387
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Codecov Report
@@ Coverage Diff @@
## master #4387 +/- ##
=========================================
+ Coverage 56.17% 56.2% +0.02%
=========================================
Files 191 191
Lines 6424 6432 +8
Branches 6 6
=========================================
+ Hits 3609 3615 +6
- Misses 2812 2814 +2
Partials 3 3
Continue to review full report at Codecov.
|
Isn't there a risk that the same test suite behaves differently on differently platforms because of symbolic links? |
I'd say it depends on the use-case. I guess you're right, that there could be a risk of that for usages where both linux and windows are supported. On the other hand, do symlinks created on linux env work on a windows env? From what I can read up the answer seems to be it depends, too, on Windows version and how the symlink is done. I'd imagine that existing usages of jest where both platforms are used don't have this issue, since symlinks aren't currently supported. What do you think or have in mind? |
|
My thinking was that if this is implemented, and some mac/linux user adds a test requiring symlinks, pushes it, and the test suite is broken for all windows developers |
|
By default nothing would break. Even if using the new flag the test requiring symlinks wouldn't be run on a windows platform. If your question ("My thinking was that if this is implemented, …") refers to implementing support for Windows, I'd again say that depends on the implementation and how symlinks would be resolved across platforms. |
|
So a windows developer won't even know that they broke a test before seeing it break on CI, and have no way of reproducing locally bar installing a new OS? |
|
Correct that a windows developer wouldn't know until the CI breaks in that scenario. However, I'd not expect a developer to install a new OS as goto solution, but investigate the CI result and be questioning why this feature was enabled and a symlink was used for a project that also have windows developers (supporting cross-platform?). I interpret your questions as that you think the the PR and/or rationale is the wrong approach. That's fine. Rather than talking about a hypothetical case back and forth I'd prefer to have a dialogue about what change you're really asking for and/or what the expectations are? |
|
All I'm saying is that introducing a feature that purposefully does not work on a platform supported by the software (I understand it might be impossible to implement) seems like a wrong decision. It's behind a flag, so maybe no big deal? I don't have strong (if any) feelings against this, as long as cross-platform support is not treated like "meh".
Don't forget that half of the developers that use nodejs are on windows1 - it's not a niche group. 1 It was last year at least, quick googling didn't give any recent numbers |
|
I have some initial thoughts and will do a more thorough review later:
Overall I almost feel like it would be better if we made crawlers pluggable – that way we can merge a PR in Jest that allows you to specify your own crawler, and you can release your own package that crawls stuff in any way you like, from any source you like. |
|
I don't think cross-platform support should be treated as a "meh" neither, sorry if it came across like that. :/ I haven't developed on/for windows platform for many years (except IE), so I simply don't feel confident about implementing support for it myself.
Ok, I'll look into this one. From what I understand the "correct" way to have this covered in the test suite would be to add a folder under integration_tests/symlinks, right?
What flag name and description would you like to have to make this "loud and clear" to be more specific?
This is an interesting idea. It feels like a potential "feature creep", though. How much effort and what are the things would have to be changed to support this? I'm not too familiar with the code base just yet to be able to tell myself. Thanks for the feedback both of you! 🙇 |
|
Just as a point of "why do this" -- I think getting something like this merged in would unstick facebook/metro#1 |
Also adds support for win32 platform
|
Implemented support in the fs API and added a integration test to have things covered. Unfortunately it turns out this doesn't work on win32 either way, as the result from Appveyor CI shows. Managed to do some troubleshooting on a windows system (v8.1), a linux symlink isn't identified a symbolic link there: // e.g. ln -s ../other.js symlink.js
fs = require('fs')
stat = fs.lstatSync('symlink.js')
stat.isSymbolicLink() // => false on windowsFound these resources that contain more details on the problems. Handling symlinks properly would require something more sophisticated, if it's even possible in all scenarios:
Closing this PR. What @cpojer suggested with making the crawlers pluggable sounds reasonable, but that would be a different PR. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
At the moment symbolic links aren't followed, because (jest-haste-map) crawler only finds regular files. Modified the
nativeFindfunction to also yield files of typel, i.e. symlinks.This resolves #1477. As suggested by @cpojerin there, I've added support behind a flag that also disabled watchman, for this to work.
Note that this change don't add support for win32 platform, since that uses a different code path/find method. I don't use Windows myself, and don't know how symlinks actually work there to be honest. My rationale here is that this change set adds value that is better than no support at all. If it would make sense win32 code path could always be added later, I figure.
Test plan
Thanks for reading!