- Notifications
You must be signed in to change notification settings - Fork306
Working for Windows#466
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
packages/api/exec.mts Outdated
| finalEnv.PATH = newPaths + (existingPath ? ';' + existingPath : ''); | ||
| } | ||
| console.log(`Executing command: ${finalCommand}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's remove the debug logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Removed
packages/api/exec.mts Outdated
| import Path from 'node:path'; | ||
| import { spawn } from 'node:child_process'; | ||
| interface NodeError extends Error { | ||
| code?: string; | ||
| } | ||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please remove these comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Okay on it
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 |
Uh oh!
There was an error while loading.Please reload this page.
| args: process.platform === 'win32' ? ['vite', ...args] : args, | ||
| env: { | ||
| ...env, | ||
| FORCE_COLOR: '1', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What's this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
..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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What's this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So on windows, we need to call the command and then addvite as an arg? Feels like this isvite vite. Surprising
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 functionspawnCall which will call:spawnCallUnix andspawnCallWindows 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let me change it then!
Fix:#297
Changes are made but requires to test on different OS.
Proof of work video here:
https://youtu.be/DC6lnsA_itU