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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use node fs module instead of execSync where possible #60

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ThoseGrapefruits
Copy link

These were the instances of execSync of some system command I found to be pretty easily replaceable with Node's fs module methods. Tests seemed to run fine with the execSync stub commented out, had to make a couple changes (http => https) to get them working but that seemed to be fixing an existing issue with the tests. It eliminated a lot of isWin ternaries, which is nice.

The reason I started in on this was that I had an issue running plain run-rs on Windows 10, which produced the following error:

Error: Command failed: del /S /Q C:\\Users\\lmooo\\AppData\\Roaming\\KnudgeDevelopment\\database\*
The specified path is not valid. 

    at checkExecSyncError (node:child_process:707:11)
    at execSync (node:child_process:744:15)
    at run (C:\Users\lmooo\Documents\GitHub\[...]\node_modules\run-rs\index.js:102:5)
    at run.next (<anonymous>)
    at onFulfilled (C:\Users\lmooo\Documents\GitHub\[...]\node_modules\co\index.js:65:19)
    at C:\Users\lmooo\Documents\GitHub\[...]\node_modules\co\index.js:54:5
    at new Promise (<anonymous>)
    at co (C:\Users\lmooo\Documents\GitHub\[...]\node_modules\co\index.js:50:10)
    at Object.<anonymous> (C:\Users\lmooo\Documents\GitHub\[...]\node_modules\run-rs\index.js:32:1)
    at Module._compile (node:internal/modules/cjs/loader:1092:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1121:10)
    at Module.load (node:internal/modules/cjs/loader:972:32)
    at Function.Module._load (node:internal/modules/cjs/loader:813:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)
    at node:internal/main/run_main_module:17:47

I tested these changes on Windows 10 (21H1, latest updates) and macOS (Mojave, latest updates).

@ThoseGrapefruits
Copy link
Author

Friendly ping on this @vkarpov15. It would be awesome to get it in soon, as I was hoping to start using this library for our local development.

@ThoseGrapefruits
Copy link
Author

Oops, closed it accidentally.

@bhumit070
Copy link

Hi @ThoseGrapefruits is this project still active?

@ThoseGrapefruits
Copy link
Author

Not sure, I never got a response on this PR (from 2 years ago now) so I just created a fork of it and rewrote the stuff I wanted to fix or have work differently. I think it works on linux-y distros but it didn’t work on windows for me and probably doesn’t handle ARM builds of mongo, as I recently added that myself.

That fork is through my company’s account so I’d have to see if it’s possible to open-source.

@bhumit070
Copy link

Hi @ThoseGrapefruits thanks for the response,
Please make it open source if possible
I want to use this package and I have laptop with m1 chipset and it does not work.

@ThoseGrapefruits
Copy link
Author

ThoseGrapefruits commented Jun 15, 2023

Hi @bhumit070,

I open-sourced our repo here: https://github.com/Knudge/mango-rs

I don't have any time to support it, but it should work ok in most cases. If you encounter a fatal problem, feel free to open an issue on that repo and I will try to find time to look at it, or you can try to tackle it yourself!

There's no npm-installable executable as of now, so you'll have to clone the repo and run it from there (instructions are in the README in that repo). Also note that many of the options that run-rs provides have been removed for simplicity, as we didn't need most of them.

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

Successfully merging this pull request may close these issues.

None yet

2 participants