Preserve symlinks when node is configured to do so

[ganemone]

Summary

This updates jest to respect the behavior of the running node process with regard to preserving symlink paths. It is compatible with both the NODE_PRESERVE_SYMLINKS environment variable and the --preserve-symlinks flag. When node is configured to preserve symlinks, jest will not resolve symlinks via fs.realpath. Additionally, this fixes some bugs where symlinks were not correctly found during fs crawling.

See: #5356

credit to @Globegitter for getting this started here: #7364. This PR is adapted from that, taking a slightly different
approach.

Test plan

Both integration tests and unit tests were added for the changes.

[ganemone]

@SimenB hopefully this addresses your comments in #7364. I have added both integration and unit tests.

[ganemone]

@SimenB what can I do to help get the conversation rolling on this PR?

[SimenB]

I'd prefer this to be an option to Jest (added to GlobalConfig) rather than every file sniffing the environment on their own. I think that'll be easier to reason about. We also need to check for --preserve-symlinks passed to node and look in NODE_OPTIONS, so just checking NODE_PRESERVE_SYMLINKS in the env isn't sufficient.

If the flag is not provided to jest, we can look at process.env somewhere in jest-config (normalize perhaps?) and default it.

[ganemone]

I'd prefer this to be an option to Jest (added to GlobalConfig) rather than every file sniffing the environment on their own. I think that'll be easier to reason about.

Hey @SimenB ,

Thanks for the response. I think it's actually much easier to reason about as an environment variable rather than jest specific configuration. Especially since this is not really configuring jest, but fixing a bug where jest didn't previously respect the expected behavior of setting this environment variable in node. This is further complicated by the fact that we need to know if preserve symlinks is set during the process of resolving and loading the jest config. Its a little weird to have jest config for loading jest config, and it makes the implementation very complicated. We also need to know about this config in several places where we don't currently have access to the config, such as static methods on the jest resolve package.

We also need to check for --preserve-symlinks passed to node and look in NODE_OPTIONS, so just checking NODE_PRESERVE_SYMLINKS in the env isn't sufficient.

I would classify that as a nice to have. Having jest at least respect one way of setting this value would be a massive improvement as it unblocks everyone who wants to use jest with bazel. This could also be added as an incremental improvement.

If you are concerned about the repetition involved here, an easy fix would be to create a new package for resolving the real path only if it matches the expected behavior of node. This could just wrap realpath-native and expose a noop if NODE_PRESERVE_SYMLINKS is set (or any of the other ways of configuring that). If you would prefer that approach, I would be happy to whip together a package for this and update the usage in jest. We could also just create it as a util in jest-util.

If you are truly hard set on using configuration for this, I would ask that you accept this PR first and update it to use configuration which defaults to the environment variable if not set. This will not introduce any breaking changes, and will help unblock a big portion of the community quickly. Please let me know your thoughts. Thank you!

[codecov-io]

Codecov Report

Merging #9732 into master will decrease coverage by 0.04%.
The diff coverage is 58.62%.

@@            Coverage Diff             @@
##           master    #9732      +/-   ##
==========================================
- Coverage   64.24%   64.20%   -0.05%     
==========================================
  Files         292      292              
  Lines       12466    12490      +24     
  Branches     3078     3089      +11     
==========================================
+ Hits         8009     8019      +10     
- Misses       3809     3815       +6     
- Partials      648      656       +8     

Impacted Files	Coverage Δ
packages/jest-cli/src/init/index.ts	83.33% <50.00%> (-1.45%)	⬇️
packages/jest-config/src/getCacheDirectory.ts	62.50% <50.00%> (-4.17%)	⬇️
packages/jest-config/src/index.ts	12.67% <50.00%> (+1.08%)	⬆️
packages/jest-config/src/normalize.ts	76.51% <50.00%> (-0.15%)	⬇️
packages/jest-core/src/runJest.ts	51.16% <50.00%> (-0.03%)	⬇️
packages/jest-resolve/src/defaultResolver.ts	74.41% <50.00%> (-1.20%)	⬇️
packages/jest-resolve/src/index.ts	44.84% <50.00%> (+0.06%)	⬆️
packages/jest-resolve/src/nodeModulesPaths.ts	82.60% <50.00%> (-3.11%)	⬇️
packages/jest-transform/src/ScriptTransformer.ts	67.25% <50.00%> (-0.16%)	⬇️
packages/jest-haste-map/src/crawlers/node.ts	83.87% <62.50%> (-2.80%)	⬇️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 649796d...4b698ad. Read the comment docs.

[ganemone]

Hey @SimenB - Have you had some time to consider my comments?

[ganemone]

Ping @SimenB - sorry to bother you. Have you had some time to consider my comments?

[SimenB]

I'm still of the opinion that to only support a single way of toggling this when node has multiple will lead to much confusion for the user - it's easier to say "we don't support it" rather than "if you do it in this way it works, but not in this way". If we only want a single way, it should be an option to Jest and not cherry picking one of the ways Node supports it

[ganemone]

I'm still of the opinion that to only support a single way of toggling this when node has multiple will lead to much confusion for the user - it's easier to say "we don't support it" rather than "if you do it in this way it works, but not in this way".

@SimenB - I've updated the PR to include support for both methods of configuring symlink preservation in node. Hopefully this will address your concerns. It is not something that can easily be put into jest config because we use this when resolving and loading the jest config.

[ganemone]

Closing in favor of #9976

[github-actions]

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.