[WIP] Windows Support #63
Conversation
Adding initial color tu Button for solve a problem with the colors in dark mode of macOs Mojave.
also change repository type from string to {type: ..., url: ...} object found in package.json and update the reducer to return an object with empty values instead of an empty string
scripts/start.js
Outdated
| // Maybe some env variable like `isWin` can be passed to the procces for easy use??? | ||
| const command = /^win/.test(process.platform) | ||
| ? `set ELECTRON_START_URL=http://localhost:${DEFAULT_PORT} && electron .` | ||
| : `ELECTRON_START_URL=http://localhost:${DEFAULT_PORT} electron .`; |
There was a problem hiding this comment.
This ternary could be avoided by passing the environment variable to exec (I believe you can pass an option object which can have an env key.
There was a problem hiding this comment.
This ternary could be avoided by passing the environment variable to exec (I believe you can pass an option object which can have an env key.
So there is indeed an option to specify env, but there appears to be a bug with Electron where not only are the values not passed, but it messes a bunch of stuff up (commands like ls or pwd fail). Discussion here.
At least, that was my experience with spawn. Given that, though, I'm happy to just have a ternary here :)
There was a problem hiding this comment.
@joshwcomeau Added a bounty on your SO question. Maybe we can get an answer for this issue there since the Github issue doesn't have any traction.
There was a problem hiding this comment.
Awesome, thanks a bunch for that! We got an answer that fixes the issue 🎉
I'm super not-clear how this will work on Windows, though. Essentially there are two proposed solutions:
- Use the
envutility instead of callingnpmdirectly. I'm gonna guess thatenvis a linux/macOS exclusive utility, so that one probably won't work? - Supply the
process.env.PATHto the command as an environment variable, to forward the current environment. Is PATH a thing on Windows?
That said, this may not even be an issue on Windows (the Electron bug with env might be MacOS only). And this is all a tangent anyway; the underlying issue is mostly about avoiding shell: true, for #25. So I'm fine if we handle this issue elsewhere.
src/reducers/paths.reducer.js
Outdated
| { | ||
| encoding: 'utf8', | ||
| } | ||
| ); |
There was a problem hiding this comment.
Does this work on macOS? If not, it should be behind a conditional.
There was a problem hiding this comment.
++, I think it should look something like:
const homedir = isWin
? getWindowsDocumentsPath()
: os.homedir();
And then getWindowsDocumentsPath would hold the child process and subsequent data munging.
|
This is looking really good. Thanks so much for your work on this! The first issue you describe, from When a project is created, it parses the const project = {
id: 'some-project',
...projectSpecificFields,
tasks: [
{
id: 'some-project-start',
projectId: 'some-project',
command: 'react-scripts start',
...otherTaskFields,
logs: [],
}
],Whenever the terminal prints to stdout, the When the project is ejected, the So I doubt this is truly a windows issue, it sounds like it should be broken for Mac, but the async series of events just happens to work. To fix it, I would just do this, in case RECEIVE_DATA_FROM_TASK_EXECUTION: {
const { task, text, isError, logId } = action;
if (task.name === 'eject' && !state[task.id]) {
// When ejecting a CRA project, the `eject` task is removed from the
// project, since it's a 1-time operation.
// TODO: We should avoid sending this action, we don't need to capture
// output for a deleted task
return state;
}it's not the most glamorous solution, but I feel confident that it's safe, and it's something we can address later on. For the second issue you found, for Gatsby dev server... if I understand, you get exactly the same error when you try to create and run a Gatsby project through the CLI, so it's not necessarily a Guppy issue? If so, I think it's fine to punt on this for now. I'm gonna set up a Windows VM and give this a test. We can open an issue with the Gatsby folks if necessary :) Thanks again for your work on this, lemme know if I can do anything else to help! |
scripts/start.js
Outdated
| // Maybe some env variable like `isWin` can be passed to the procces for easy use??? | ||
| const command = /^win/.test(process.platform) | ||
| ? `set ELECTRON_START_URL=http://localhost:${DEFAULT_PORT} && electron .` | ||
| : `ELECTRON_START_URL=http://localhost:${DEFAULT_PORT} electron .`; |
There was a problem hiding this comment.
This ternary could be avoided by passing the environment variable to exec (I believe you can pass an option object which can have an env key.
So there is indeed an option to specify env, but there appears to be a bug with Electron where not only are the values not passed, but it messes a bunch of stuff up (commands like ls or pwd fail). Discussion here.
At least, that was my experience with spawn. Given that, though, I'm happy to just have a ternary here :)
src/middlewares/task.middleware.js
Outdated
| // For Windows Support | ||
| // Windows sends code 1 (I guess its because we foce kill??) | ||
| const successfullCode = isWin ? 1 : 0; | ||
| const wasSuccessful = code === successfullCode || code === null; |
There was a problem hiding this comment.
Hm, I think the issue here is that we lose the ability to detect true failures from false ones; if the task crashes, it would still display "Task Completed" for Windows.
That said, I don't see an easy solution to the problem, it doesn't look like taskkill supports specifying the exit code, and the current architecture means there's no clear way to communicate.
I don't think this is a showstopper; if we can get everything else working on Windows, we can deal with fixing this issue in a subsequent PR (likely by rearchitecting it so that ABORT_TASK handles this directly, instead of using the child-process listeners as an event bus)
There was a problem hiding this comment.
I think following up state of the process together with a custom redux variable might be beneficial. We can check if the termination was triggered with the switch or the process ended unexpectedly.
I don't know how much of this can be implemented but there can be lots of different informative states. For example rather than just having Running/Idle/Failed we can maybe have Started/Running/Stopping/Stopped/Idle/Failed/Ended Unexpectedly. Since Guppy targets new users, being more informative IMO a great approach.
src/middlewares/task.middleware.js
Outdated
| const wasSuccessful = code === 0 || code === null; | ||
| // For Windows Support | ||
| // Windows sends code 1 (I guess its because we foce kill??) | ||
| const successfullCode = isWin ? 1 : 0; |
There was a problem hiding this comment.
Nit: typo, should be successfulCode (only 1 l)
src/middlewares/task.middleware.js
Outdated
| switch (projectType) { | ||
| case 'create-react-app': | ||
| return [`PORT=${port} npm`, 'run', task.name]; | ||
| // For Windows Support | ||
| command = isWin ? `set PORT=${port} && npm` : `PORT=${port} npm`; |
There was a problem hiding this comment.
Not urgent, but I think it'd be neat if we had a helper method, like formatCommandForPlatform, that looked like:
command = formatCommandForPlatform('npm`, { PORT: port });
That way, the whole set prefix (and even the whole prefixing with env variables) wouldn't need to be understood by devs making changes to any part of the code that starts commands.
Feel free to just add a TODO though, I can tackle this in a subsequent PR :)
src/reducers/paths.reducer.js
Outdated
| { | ||
| encoding: 'utf8', | ||
| } | ||
| ); |
There was a problem hiding this comment.
++, I think it should look something like:
const homedir = isWin
? getWindowsDocumentsPath()
: os.homedir();
And then getWindowsDocumentsPath would hold the child process and subsequent data munging.
src/reducers/paths.reducer.js
Outdated
| } | ||
| ); | ||
| // prettier-ignore | ||
| const winDocumentsPath = winDocumentsRegRecord.split(' ')[winDocumentsRegRecord.split(' ').length - 1].replace('%USERPROFILE%\\', '').replace(/\s/g, ''); |
There was a problem hiding this comment.
Do you happen to know how consistent windows registry formats are between versions of windows? Would this work from Windows 7 through to Windows 10, for example?
IIRC registry stuff tends not to change much, so I believe it's safe, but I haven't used Windows in over a decade 😅so my info is a bit rusty
There was a problem hiding this comment.
I changed it a little bit. Initially split was creating bunch of empty values, that is why I replaced bunch of spaces. With single space split it should work in prior windows versions too. I'm going to try it on Win7 soon.
| @@ -11,6 +11,9 @@ import type { ProjectType } from '../types'; | |||
|
|
|||
| const fs = window.require('fs'); | |||
| const childProcess = window.require('child_process'); | |||
| const path = window.require('path'); | |||
| const os = window.require('os'); | |||
| const isWin = /^win/.test(os.platform()); | |||
There was a problem hiding this comment.
I think it'd be great to add a services/platform.services.js file that would export:
isWin, this quick checkgetWindowsHomeDir, mentioned aboveformatCommandForPlatformalso mentioned above.
This is super non-urgent, I'm happy to go in after this merges and do it myself :) just mentioning it in case you're up for it (and so that I don't forget)
There was a problem hiding this comment.
I moved isWin and getWindowsDir to platform.services.js. I was going to also propose a complete formatCommad service but I thought it's not really just for windows support so I think it should be in a separate PR. Using a global service function to get the command can benefit from future library additions like next. Doesn't require to change things on different files for example.
| // For Windows Support | ||
| // To support cross platform with slashes and escapes | ||
| // Taken from https://github.com/AWolf81/guppy/commit/b2434d907d0c2c2585006d82ef14523a974de6a0 | ||
| const projectPath = path.join(parentPath, id); |
| // Finds if the specified port is in use | ||
| const command = /^win/.test(os.platform()) | ||
| ? `netstat -aon | find "${port}"` | ||
| : `lsof -i :${port}`; |
There was a problem hiding this comment.
Yay! Glad to see an equivalent exists (looks like it goes all the way back to Vista too, nice)
Fix docs { outside of Guppy }
|
@joshwcomeau First of all No Problem. And Second of all Thank you for the great project. I'm happy to help. I want to do that |
…b-link fix joshwcomeau#48: get the repo url from package.json
Add an initial color to Button
add build step for linux
There was a problem hiding this comment.
@bennygenel went through and the latest changes look good!
One thing I haven't seen dealt with in this PR yet is the issue outlined in #25 - we're using echo right now to hack around the fact that childProcess tasks are uni-directional in terms of communication, but we need to accept the prompt for the CRA eject script.
It seems like this wasn't an issue after all? Since you've mentioned being able to eject projects (you get an error, but it happens after the prompt is confirmed).
I want to do that formatCommandForPlatform but I think there should be a settlement on how to approach this for future proof things. I'll be happy to hear what you have in mind for this.
Good question - AFAIK, commands work the same way on MacOS and Linux, so really I think this method could just look like:
const formatCommandForPlatform = (command, environmentVars) => {
// Create a string like `PORT=4545 HI=5`
// Could also use Object.values but then Flow will complain x_x
const envVarString = Object.keys(environmentVars)
.map(key => `${key}=${environmentVars[key]}`
.join(' ');
return isWin
? `set ${envVarString} ${command}`
: `${envVarString} {command}`;
}It's entirely possible that this is not a future-proofed solution, but I think that's OK, since it's self-contained; the API (as in, the arguments and the return type) shouldn't change if we determine that we need to distinguish between Mac/Linux, since the platform info is available globally.
But yeah, I don't have strong opinions here, so feel free to push back if you have a different idea!
I somehow keep running out of time, so I haven't been able to run this in a VM and test Gatsby yet. Sorry for the delay. I'll try and squeeze it in this evening (I got a VM downloaded, but not yet installed/configured)
scripts/start.js
Outdated
| // Maybe some env variable like `isWin` can be passed to the procces for easy use??? | ||
| const command = /^win/.test(process.platform) | ||
| ? `set ELECTRON_START_URL=http://localhost:${DEFAULT_PORT} && electron .` | ||
| : `ELECTRON_START_URL=http://localhost:${DEFAULT_PORT} electron .`; |
There was a problem hiding this comment.
Awesome, thanks a bunch for that! We got an answer that fixes the issue 🎉
I'm super not-clear how this will work on Windows, though. Essentially there are two proposed solutions:
- Use the
envutility instead of callingnpmdirectly. I'm gonna guess thatenvis a linux/macOS exclusive utility, so that one probably won't work? - Supply the
process.env.PATHto the command as an environment variable, to forward the current environment. Is PATH a thing on Windows?
That said, this may not even be an issue on Windows (the Electron bug with env might be MacOS only). And this is all a tangent anyway; the underlying issue is mostly about avoiding shell: true, for #25. So I'm fine if we handle this issue elsewhere.
…istingProjectError Add action to make unsupported project errors fail gracefully
|
@bennygenel I just sent an invitation to become a collaborator on this repo :) I really appreciate the work you've done so far on adding Windows support, + being involved in discussions on other issues. This is still a super new project so the conventions are still being established, but my current thinking is that collaborators can take a more active role in triaging/closing issues and reviewing/merging PRs (I think for the time being I'd like to retain control over accepting/rejecting any change with UI implications, but feel free to approve and merge PRs that are logic-only and relatively small). Please just make sure that myself or another collaborator has approved your own code before merging it :) And of course, this doesn't come with any additional expectation on my part, in terms of workload or commitment. Feel free to do as much or as little as you'd like. I'm in the middle of installing a Windows VM so I hope to be able to test this stuff soon! |
* Tests for dependencies reducer * replace toMatchShapshot with toMatchInlineSnapshot, update jest
|
Hi @bennygenel Awesome, sounds like you're making good progress! For the But yeah, I think this problem, while interesting, isn't pressing. That can be revisited in the future, for now we can punt on it and not bother with Adding For the NPX issue, I wonder if we should just do So, I got my windows VM set up, installed Git and Node and VS Code. When I tried to install the Guppy dependencies, though, I got an error: I haven't tried to debug this at all yet, but I thought I'd ask and see if this was something you ran into? If not, don't worry about it, I'll investigate more. |
|
@joshwcomeau have you tried to delete |
* Add support for highlighting results * Use DOMParser instead of dangerouslySetInnerHTML and modify highlight styles * Better styling for spacing, and add more comments * Wrap part.value in <span> to keep spacing * Changes to PR to complete review - Move CustomHighlight to its own component - Move safeEscapeString into its own utility function - Don't run the highlight on the package name
A few changes to the contribution docs: - Rearrange so that "Sending a Pull Request" is closer to the top - Add a "most important" blurb in the intro - Add a "Collaborators" section
Add icon instructions. Reorganize dev docs
…joshwcomeau#82) * Fix html display in dependency search, and styling tweaks for results * Hide 'load more' on disabled, display as props for ExternalLink * Set default display prop to inline-block, remove curly braces for string prop
* Add issue templates Create two templates: one for Bug reports, one for Feature requests. * Replace multiple issue templates with 1. Add PR template * Update environment info * Add HTML comments
Split off the issue created in joshwcomeau#94.
* add license and code of conduct - updated readme to reflect these changes * update license format, add license to package.json
…into windows-support modified: scripts/start.js modified: src/middlewares/task.middleware.js
|
Yay, I got my local environment running, and was able to give this a try. Building a project worked flawlessly!! I did hit an error once I tried running the dev server, after creating a new create-react-app app: Let me know if I can provide any other info! |
|
@joshwcomeau I also had that issue yesterday but I couldn't find the location in code where it uses I'll have to check what the method |
|
@joshwcomeau @AWolf81 what's the output of from within the shell you're using to run Guppy? It should output something like At first glance it seems like this error is resulting from |
|
@superhawk610 @AWolf81 @joshwcomeau The error is because of |
Speaking with @bennygenel on Gitter, he says that this is only an issue with NPM, not with Yarn. If it proves too troubling, maybe we should just tackle #35 first, moving to use Yarn. There are a bunch of other reasons to move to Yarn, as well. I mentioned in Gitter, but my schedule for the next 3 weeks is such that I can't spare much time to work on Guppy (pretty much only participating in discussions like these). If anyone wants to tackle this in the meantime, that'd be awesome 🙏 but otherwise I can have it at the top of my list when my schedule frees up. |
|
Closed via #125. |
Created PR to better track down changes from #27
So far nothing changed too much.
cross-envfor easier cross-platform developmentpath.join(from @AWolf81 fix).cmdto commands to make sure they run correctlyset VARIABLE_NAME & commadpattern for commands that requireenvvariables%USERPROFILE%\Documentsis -IMO- much better place for Windows. This can change if decided of course.ISSUES:
create-react-appproject I got an error. I'm not sure why. Any help is appreciated.TODOS:
Any todos can go here...