search

Jarred-Sumner / git-peek

git repo to local editor instantly
MIT License
722 stars 17 forks source link

rmSync is not a function #14

Closed dscho closed 3 years ago

dscho commented 3 years ago

I get this stack trace on Windows:

$ git peek https://github.com/git-for-windows/git/pull/3017
 Extracting repository to temp folder...
💻 Launched editor in 0.89s
 Finished downloading repository!
🗑  Deleted temp repo
TypeError: u3.default.rmSync is not a function
    at uE (C:\Users\me\AppData\Roaming\npm\node_modules\@jarred\git-peek\bin\git-peek:497:846)
    at s3.run (C:\Users\me\AppData\Roaming\npm\node_modules\@jarred\git-peek\bin\git-peek:528:2695)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
TypeError: u3.default.rmSync is not a function
    at uE (C:\Users\me\AppData\Roaming\npm\node_modules\@jarred\git-peek\bin\git-peek:497:846)
    at s3.run (C:\Users\me\AppData\Roaming\npm\node_modules\@jarred\git-peek\bin\git-peek:528:2695)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

That's with

$ node -v
v12.16.2

Is it possible that https://github.com/Jarred-Sumner/git-peek/blob/885e324360f00ff030f44fcd99f4ea29f98b459b/src/index.ts#L99 should use await fs.promises.rm() instead (of course, doExit() would then have become async and all the call sites, including this one would have to be adjusted).

Jarred-Sumner commented 3 years ago

https://nodejs.org/api/fs.html#fs_fs_rmsync_path_options Looks like fs.rmSync was added in v14. I’ll add a polyfill (you’d be welcome to submit a PR though if you’d like). I do this already with Promise.any

This function call should be synchronous because it may run immediately before process exit, from one of the kill signals. I don’t think we can guarantee that the next tick will be invoked before the process ends. That specific function call should only happen when the tmp module doesn’t properly clean up.

dscho commented 3 years ago

https://nodejs.org/api/fs.html#fs_fs_rmsync_path_options Looks like fs.rmSync was added in v14. I’ll add a polyfill (you’d be welcome to submit a PR though if you’d like). I do this already with Promise.any

Would love to, but ENOTIME :-(

This function call should be synchronous because it may run immediately before process exit, from one of the kill signals. I don’t think we can guarantee that the next tick will be invoked before the process ends. That specific function call should only happen when the tmp module doesn’t properly clean up.

Ah, that makes sense. TBH I wouldn't even know how to provide that polyfill. However, it might be much, much easier than that: https://nodejs.org/api/fs.html#fs_fs_rmdirsync_path_options

Jarred-Sumner commented 3 years ago

However, it might be much, much easier than that: https://nodejs.org/api/fs.html#fs_fs_rmdirsync_path_options Thanks.

Fixed in v1.1.29. Though I honestly didn't test that it runs successfully on Node 12, but the diff is so small it seems unlikely to break it.

dscho commented 3 years ago

Fixed in v1.1.29

BTW I don't see either v1.1.29 nor v1.1.28 in https://github.com/Jarred-Sumner/git-peek/releases...

Jarred-Sumner commented 3 years ago

Yeah the release script broke, I'm fixing it.