fix: use symlinkDir

[aminya]

Symlinking was always falling on my computer, but symlink-dir library does the job (e.g. it handles recursive symlinks). The tests now start pretty fast.

Fixes AtomLinter/linter-eslint#1422 (comment)

[UziTech]

It looks like they just use junction instead of dir for windows. If this works for you we could avoid importing a whole dependency.

Suggested change

	await symlinkDir(userPack, loadablePack);
	const type = process.platform === "win32" ? "junction" : "dir";
	await fs.symlink(userPack, loadablePack, type);

[UziTech]

The rest of that package is error handling when the folder already exists, but we don't have to worry about any of that since this will always be in a new temp folder.

[aminya]

Duplicating the functionality of that package here means you need to add tests, verify that it works on all platforms, do error handling, etc. But using it as it is means it works without issues. This package is the package used inside pnpm.
Not sure if a avoiding this a few KB of code helps the project

[UziTech]

I don't really see the need for new tests since the change would be so minimal. Literally this one line of code is all that is needed. Importing more dependencies means increasing the security risk. In this specific situation it doesn't seem necessary.

If we add the dependency we should add tests to make sure updating it doesn't break something in the future. This one line of code will most likely never need to be updated.

If we needed the error handling that would be one thing, but as I stated above we don't.

[aminya]

I tried your suggestion, but many of the linter-eslint tests start to fail now.

With symlink-dir everything works

[UziTech]

why are they failing? What error are they giving? I don't think it is because of that change because as I said that is all the dependency does.

[aminya]

With symlink dir, it correctly creates links:

With your suggestion, it creates some empty files and the tests fail

The error:

TypeError [ERR_INVALID_CALLBACK] [ERR_INVALID_CALLBACK]: Callback must be a function. Received 'junction'
    at makeCallback (fs.js:150:11)
    at Proxy.symlink (fs.js:881:20)
    at file:///C:/Users/aminy/Documents/GitHub/JavaScript/atom-jasmine3-test-runner/lib/create-runner.js:142:16
    at Generator.next (<anonymous>)
    at file:///C:/Users/aminy/Documents/GitHub/JavaScript/atom-jasmine3-test-runner/lib/create-runner.js:290:22
    at Generator.next (<anonymous>)
    at step (file:///C:/Users/aminy/Documents/GitHub/JavaScript/atom-jasmine3-test-runner/lib/create-runner.js:83:37)
    at processTicksAndRejections (internal/process/task_queues.js:89:5) {
  rawStack: [
    CallSite {},
    CallSite {},
    CallSite {},
    CallSite {},
    CallSite {},
    CallSite {},
    CallSite {},
    CallSite {}
  ]
}

Actually, this issue is gone now.
steelbrain/package-deps#334

[UziTech]

I see. This is an issue with the way fs-plus makes fs async.

[UziTech]

try:

const type = process.platform === "win32" ? "junction" : "dir";
// eslint-disable-next-line no-sync
fs.symlinkSync(userPack, loadablePack, type);

[UziTech]

The code will not speed up by using async functions since there will not be any other operations on the thread while it is waiting.

[UziTech]

It looks like fs-plus does not return promises so I don't think this will work. It requires a callback to be async.

[aminya]

Could you push the edits yourself?

[github-actions]

🎉 This issue has been resolved in version 5.2.3 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀