Windows support

[joshwcomeau]

Guppy currently only works on MacOS. Let's fix that and make it Windows-friendly!

Note: This ticket is blocked by a couple of others that should probably be tackled first. See below:

Some notes:

• Guppy does a lot of filesystem stuff, and it's unclear to me how much work is required for Windows compatibility.

• All projects are currently created in {os.homeDir()}/guppy-projects. Does this still make sense on Windows?

• This ticket is blocked by Bi-directional communication with processes #25, because we're currently hacking around an issue by using echo, which isn't supported on Windows.

• This ticket is also sorta blocked by Switch to Electron Builder for auto-updates and proper installers #26 - if we're gonna switch to electron-builder, we should do that before spending time/energy on a windows installer solution.

[geekysrm]

What about Linux?

[joshwcomeau]

What about Linux?

Good point, I should create an issue for that. Truthfully it's not as high on the priority list for me (given that Guppy is aimed at beginners, I'm guessing there's a far bigger Windows audience than Linux), but it would still be great to have, and it's probably not that much effort to add support for it.

[bennygenel]

@joshwcomeau I tried to run project on a Windows 10 computer to see how much of a change does project need to run in reality. I don't know if these going to help or not but I guess it's a start.

All projects are currently created in {os.homeDir()}/guppy-projects. Does this still make sense on Windows?

os.homeDir() resolves to C:\Users\username on windows. Usually default path for projects is C:\Users\username\Documents (Most of the IDEs including Github desktop app use this path). So it's a quick fix if it needs to change. I didn't mind it but it would be nice to select a default path.

What I achieved so far:

• Successfully started guppy
• Successfully created a new react project
• Successfully imported an already created project
• Installed a new package to a created project
• Successfully built a project
• Successfully run the development server

What I have changed:

• Added cross-env package as a devDependency so it is easier to set env variables for development in windows. After that I changed below scripts.

"start": "cross-env NODE_ENV=development npm run start:react",
"start:react": "cross-env BROWSER=none node scripts/start.js",
"package": "cross-env GENERATE_SOURCEMAP=false npm run build && npm run package:mac",

• Added cross-env to the electron starter command. This might need to change since cross-env is only available in development. I don't know if it makes sense to have it as a regular dependency.

  exec(`cross-env ELECTRON_START_URL=http://localhost:${DEFAULT_PORT} electron .`,
    (err, stdout, stderr) => {
      // ....
    }
  );

• Added cross-env to dev packager command. Same issue with the ELECTRON_START_URL

  switch (projectType) {
    case 'create-react-app':
      return [`cross-env PORT=${port} npm`, 'run', task.name];
    case 'gatsby':
      return ['npm', 'run', task.name, '--', `-p ${port}`];
    default:
      throw new Error('Unrecognized project type: ' + projectType);
  }

• Added os.platform() check to modify create new app command

  const npx = /^win/.test(os.platform()) ? 'npx.cmd' : 'npx';
  switch (projectType) {
    case 'create-react-app':
      return [npx, 'create-react-app', path];
    case 'gatsby':
      return [npx, 'gatsby', 'new', path];
    default:
      throw new Error('Unrecognized project type: ' + projectType);
  }

• Added os.platform() check to port finding function

    const checkPort = (port = 3000, attemptNum = 0) => {
      // windows command from https://stackoverflow.com/a/26030523/2315280
      const command = /^win/.test(os.platform()) ? `netstat -aon | find "${port}"` : `lsof -i :${port}`;
      childProcess.exec(command, (err, res) => {
          // .....
        }

        resolve(port);
      });
    };

If these changes make sense please ping me. I'll be happy to submit a PR. I did not test gatsby since I don't have any experience with it but with similar changes I thing it should work too.

[AWolf81]

@bennygenel I did the same in a branch here.

I've also added cross-env at the positions that you've mentioned.

For the issue with npx.cmd I've used cross-spawn so I'm not getting ENOENT. I wasn't aware that it was just not finding it because of the .cmd. Not sure which version is better.

At the moment, I'm a bit stuck at stopping the development server. When I'm stopping the server the GUI displays:

And re-starting is not working properly. I have to reload the view and then I can restart the server.
In task.middleware.js I've changed inside case ABORT_TASK the following code and using tree-kill for task killing:

psTree(processId, (err, children) => {
    if (err) {
      console.error('Could not gather process children:', err);
    }

    const childrenPIDs = children.map(child => child.PID);

    console.log('pstree', processId, children, err);
    //childProcess.spawn('kill', ['-9', ...childrenPIDs]);
    for (let pid of childrenPIDs) {
      if (pid) {
        console.log('kill', pid);
        // pid is defined
        kill(pid);
      }
    }

    ipcRenderer.send('removeProcessId', processId);

    // Once the children are killed, we should dispatch a notification
    // so that the terminal shows something about this update.
    // My initial thought was that all tasks would have the same message,
    // but given that we're treating `start` as its own special thing,
    // I'm realizing that it should vary depending on the task type.
    // TODO: Find a better place for this to live.
    const abortMessage = isDevServerTask(name)
      ? 'Server stopped'
      : 'Task aborted';

    console.log('abort msg', abortMessage);
    next(
      receiveDataFromTaskExecution(
        task,
        `\u001b[31;1m${abortMessage}\u001b[0m`
      )
    );
  });

The killing of the processes seems to work. I'm just not sure why restarting is not working. Also there are many processes running for the dev server.
Is there a way to see which processes are created at the start of the server?

[bennygenel]

@AWolf81 fixed restarting dev server problem. You can check the PR for details. It was related to exit code check on file task.middleware.js.

[joshwcomeau]

Wow, so glad to see so much progress on adding Windows support! Y'all rock.

I'm reviewing @bennygenel's PR now. @AWolf81 it looks like you were both going in a very similar direction, but it'd be awesome if you two could work together to find the optimal solution (rather than have 2 competing PRs, I'd love to see a collaboration on 1).

Thanks so much!

[AWolf81]

@joshwcomeau Sure, I've quickly reviewed @bennygenel PR and it looks great. I'll try to help him with open issues at eject and the Gatsby topic.

Restarting issue
OK, noticed. Not sure why the exit code is different for Windows. I'll have a look later. Maybe that could be a permission issue or the forcing is affecting the error code. It should be exit code 0 also in Windows.

[bennygenel]

@AWolf81 and @joshwcomeau Thank You. I'm happy that you liked the progress. I'm going through your reviews and fixing quick ones.

[joshwcomeau]

Delighted that this has been completed, and shipped in 0.2.0 :D