Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

rmSync is not a function #14

Closed
dscho opened this issue Feb 12, 2021 · 5 comments
Closed

rmSync is not a function #14

dscho opened this issue Feb 12, 2021 · 5 comments

Comments

@dscho
Copy link

dscho commented Feb 12, 2021

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

fs.rmSync(instance.destination, {
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
Copy link
Owner

https://nodejs.org/api/fs.html#fs_fs_rmsync_path_options Looks like fs.rmSync was added in v14. I鈥檒l add a polyfill (you鈥檇 be welcome to submit a PR though if you鈥檇 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鈥檛 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鈥檛 properly clean up.

@dscho
Copy link
Author

dscho commented Feb 12, 2021

https://nodejs.org/api/fs.html#fs_fs_rmsync_path_options Looks like fs.rmSync was added in v14. I鈥檒l add a polyfill (you鈥檇 be welcome to submit a PR though if you鈥檇 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鈥檛 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鈥檛 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
Copy link
Owner

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
Copy link
Author

dscho commented Feb 12, 2021

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants