fix: avoid endless loop when handling symlinks (#109) by @q0rban

[q0rban]

This resolves #108 for me. I tried to run tests locally to see if this breaks anything, but I guess the tests need to run in CI as they were failing with or without this change on my environment.

Fixes #108

[robertsLando]

@q0rban I'm not sure that just removing code is the correct way to handle this. You should handle the case of infinite loop like keeping track of paths to be sure you don't hit the same path twice to break the loop

[q0rban]

The interesting thing is that the code is already keeping track of the paths. I couldn't figure out a scenario where this code wouldn't result in an infinite loop, which is why I deleted it. But then I realize it is assuming that the paths are not nested at the same level. So I believe the code will work if path/to/some/symlink/index.js is symlinked to path/to/real/index.js (since they have different nesting levels), but I believe it will always loop if path/to/symlink/index.js is linked to path/to/real/index.js since they have the same nesting depth.

Furthermore, I don't believe this function will properly handle the scenario where there are multiple symlinks involved. For example, let's say module chicken is symlinked wholesale into another module:

path/to/node_modules/some-module/node_modules/symchicken -> path/to/node_modules/some-module/node_modules/chicken

but then, what happens if chicken has some links in it as well?

chicken/lib/link.js -> chicken/deeply/nested/index.js

This would result in

path/to/node_modules/some-module/node_modules/symchicken/lib/index.js -> path/to/node_modules/some-module/node_modules/chicken/deeply/nested/index.js

Does that mean this logic needs to recurse? The mind reels.

[q0rban]

Incidentally, do you have any tips on running tests locally? I don't see any documentation on that in the repo.

[q0rban]

@robertsLando updated the approach and would appreciate your thoughts. The existing logic seems error prone, and thus difficult to ascertain the intent of it.

[robertsLando]

See https://github.com/yao-pkg/pkg/pull/111/files

[robertsLando]

Also please fix lint issues: yarn fix

[robertsLando]

Could you also add a test that covers this?

test-99-#108

[q0rban]

Could you also add a test that covers this?

test-99-#108

Whoops, apologies, I missed this. Will take a look at what this entails.

[robertsLando]

See review above

[q0rban]

Okay, test added. I can confirm that the test just hangs on the main branch, and builds properly on this branch.

[q0rban]

See review above

I know, I requested another review after pushing fixes based on that review. :)

[robertsLando]

Thanks for this! LGTM :)

[robertsLando]

Found improvements on how test works? 😆 Spent 2 days trying to improve them