Conversation
packages/api/exec.mts
Outdated
| finalEnv.PATH = newPaths + (existingPath ? ';' + existingPath : ''); | ||
| } | ||
|
|
||
| console.log(`Executing command: ${finalCommand}`); |
There was a problem hiding this comment.
Let's remove the debug logs
packages/api/exec.mts
Outdated
| code?: string; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Please remove these comments
|
Removed Debugging console & Comments made from my side |
|
The updated code improves maintainability, efficiency, and cross-platform compatibility by centralizing executable resolution into a reusable |
| args: process.platform === 'win32' ? ['vite', ...args] : args, | ||
| env: { | ||
| ...env, | ||
| FORCE_COLOR: '1', |
There was a problem hiding this comment.
..env is for copying all the object, for creating in the following object although I added FORCE_COLOR: '1' Because it was getting difficult to find the response so for making it shine I added, I will remove it if you want
| args: process.platform === 'win32' ? ['vite', ...args] : args, | ||
| env: { | ||
| ...env, | ||
| FORCE_COLOR: '1', |
There was a problem hiding this comment.
For just making sure I can track that this log can easily noticeable while I was debugging, if you want I will remove FORCE_COLOR it was for me to make sure that it is logging
| command: | ||
| process.platform === 'win32' ? 'npx.cmd' : Path.join(cwd, 'node_modules', '.bin', 'vite'), | ||
| cwd, | ||
| args: process.platform === 'win32' ? ['vite', ...args] : args, |
There was a problem hiding this comment.
So on windows, we need to call the command and then add vite as an arg? Feels like this is vite vite. Surprising
There was a problem hiding this comment.
Have to hard code it, only then it's running otherwise vite is not getting started, atleast for me it havn't
There was a problem hiding this comment.
Alien, Yes! While Vite code which was already there didn't started vite and thrown error, require to hard code this in the following way
packages/api/exec.mts
Outdated
| export function spawnCall(options: SpawnCallRequestType) { | ||
| const { cwd, env, command, args, stdout, stderr, onExit, onError } = options; | ||
| const child = spawn(command, args, { cwd: cwd, env: env }); | ||
| class ExecutableResolver { |
There was a problem hiding this comment.
I'm a bit scared of this change. It's a pretty big refactor of an important part of the app.
I'm wondering if we can remove the new class implementation, and instead have some simple function based routing.
So first run a function spawnCall which will call:
spawnCallUnix and spawnCallWindows based on the process.platform switch you do line 56.
That way the mac / unix code can stay the exact same which I think is safeer, and windows can have it's own code path.
There was a problem hiding this comment.
Let me change it then!
2 participants
Fix: #297
Changes are made but requires to test on different OS.
Proof of work video here:
https://youtu.be/DC6lnsA_itU