Windows support #125

AWolf81 · 2018-08-11T22:46:16Z

Related Issue: #27
Related PR: #63

Summary:
Created a new PR based on @bennygenel windows-support branch and my npm-to-yarn branch. I couldn't change the target of the other pull-request. @bennygenel I hope this is OK that I've created it. Your PR is OK but wasn't at the right base. Also my PR to your fork is not needed anymore - I'll check if I can close it with-out merging.

So I created a new branch and merged both branches. Now we're able to merge easily.

Please review & test with Mac and Linux. I could also do the Linux review in a virtual machine if there is no dev. with Linux.

Review infos:

Windows support branch: Review pretty much covered in the other PR. The following changes were added:
- Added cross-env for env. handling at execution
- .cmd formatting
- Modified findAvailablePorts
- Modified default project dir.
npm-to-yarn branch: Not reviewed yet.
- Added yarn as project dependency.
- Changed package manager cmd to yarn
- Added app config to src/config/app.js (needs to be discussed - my thoughts see below)
- Changed spawn process to use promises & preparation for debug logging to console

Some thoughts:

Application-wide configuration: There is no place where we can add not user changable app configuration. That's why I addeed it here. We could also add Agolia API keys here and move them from src/constants. Later we could also enable / disable debug logs with this file.
Spawn process method: I need to check if it makes sense to move this method to a service or we could replace it with child-process-promise to have promises for spawn. I think it is OK to keep the method like it is - maybe moving to a service makes sense if spawn process is used at many location.

Issues:

After ejecting the devserver is not starting and fails with babel-loader is missing. Babel-loader is not available in the dependencies list but it is in the package.json file. I have to check what happens during eject & why this dependency is missing. But I think that's probably not Windows related and we could fix this later - only if there is no quick fix.
Gatsby project are there any problems?

…into windows-support modified: scripts/start.js modified: src/middlewares/task.middleware.js

…pport

joshwcomeau

Yayy, thanks so much for commandeering this @AWolf81! Hope this is ok @bennygenel, but it's important that we get this merged soon :)

I'll give this a full review soon, but one quick thing I noticed: When running Guppy on a Mac, I get this issue:

joshwcomeau · 2018-08-12T14:06:34Z

Ahhh, the issue is isWin; MacOS is named darwin, so the regex passes!

Looks like we'd need to make sure it starts with win? Or just make sure it includes win, but not darwin.

AWolf81 · 2018-08-12T20:54:29Z

src/services/create-project.service.js

+  // For Windows Support
+  // Windows tries to run command as a script rather than on a cmd
+  // To force it we add *.cmd to the commands
+  const command = isWin ? 'npx.cmd' : 'npx';


Line 147 requires a function call as well. isWin(). I'll add it.

Changed this to formatCommandForPlatform.

AWolf81 · 2018-08-12T21:19:35Z

I've tested the branch with Ubuntu and noticed the following issues:

Starting the devServer & Electron is not working as it is. I'm not sure how to fix it. For now I've commented env object passed to exec in scripts/start.js and modified the called command to ELECTRON_START_URL=http://localhost:${DEFAULT_PORT} electron${/^win/.test(process.platform) ? '.cmd' : ''} . With this Electron is starting.
After start I've noticed that the menu bar is not displayed. See the following screenshot

Update: OK, I noticed that the menu bar is always on top of the desktop and not at the window. Probably not changeable & I'm just not used to it. Atom text editor is with the same behaviour - so this is no issue. See this screenshot.
DevServer not starting it displays an error message (see screenshot above). It tells yarn requires Node.js 4.0 or higher. Installed node version is v8.11.1 (output from node -v). Not sure where yarn is getting this wrong information. Added NODE_PATH to terminal env. but still the same message. After commenting env object in task.middleware it starts but complains that port 3000 is already open - seems like findAvailablePorts is not working as expected.
Eject not working. It closes after prompt with-out calling yarn eject. No added dependencies. Just the state changes and it is hiding the eject button.

Working:

Create new react app (Gatsby not tested yet)
Add & Remove dependency
Importing projects

Unit tests:
Test are failing because remote.app is undefined. This is no Ubuntu issue because this happens also with Windows. I have to check why it fails. TypeError: Cannot read property 'app' of undefined

Summary:
I think we'll need a separate issue to fix the app for Ubuntu. Should we re-open Linux Support issue #45 ? After fixing the isWin() line in create-project I think it's OK for now. I've found some smaller improvements that I'll also add - some unused imports.
I'll copy the above info to an issue so we can track this.

@patcito I saw that you tested Ubuntu before. Could you please checkout the windows-support branch to see if you're facing similar problems? Just to be sure that it is not related to my VM ware setup.

superhawk610

Great job, everybody. This passes a basic test suite across Windows/Mac/Linux, I think it's ready to ship.

mathieudutour

The caching looks good.
There is one more thing: the capitalisation of labels is different on Windows than on MacOS:

Button Label on Mac
Button label on windows

It’s a detail and maybe we don’t care, just wanted to point it out

superhawk610 · 2018-08-15T19:10:53Z

@mathieudutour which button labels are you referring to?

mathieudutour · 2018-08-15T19:11:50Z

That was just an example. Every buttons, label and menu item follow this rule

superhawk610 · 2018-08-15T19:19:48Z

They all look the same cross-platform to me - View Details, File > Create New Project, File > Import Existing Project, Add To Project...

EDIT - What version of Windows are you using? I'm using Windows 10.

mathieudutour · 2018-08-15T19:26:37Z

I'm not using windows but this issue is a pretty big deal over at desktop/desktop (one commit for example: desktop/desktop@eb806bd)

superhawk610 · 2018-08-15T19:30:28Z

Ahhh, I misunderstood. I'll correct those and add in a couple accelerators.

AWolf81 · 2018-08-15T20:00:50Z

src/middlewares/task.middleware.js

-          env: {
-            ...window.process.env,
-          },
+          env: getBaseProjectEnvironment(projectPath),


@superhawk610 I think it's OK to add the path to ./node_modules/.bin here but I think that shouldn't be needed. Maybe we can check this later and leave this for now.

It should be enough to have cwd as it is like cd to project directory and run yarn inside that folder. Then yarn will look into ./node_modules/ for the file to execute and if is not finding the file to executable it will look if it's globally available.

Without explicitly providing the .bin path, I get some form of this error on Windows:

$ react-scripts test --env=jsdom --coverage 'react-scripts' is not recognized as an internal or external command, operable program or batch file. error Command failed with exit code 1. Task completed info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

It may be a Git Bash-specific issue.

AWolf81 · 2018-08-15T20:03:30Z

package.json

@@ -39,6 +39,7 @@
    "electron-store": "2.0.0",
    "fix-path": "2.1.0",
    "gatsby-cli": "1.1.58",
+    "ps-tree": "^1.1.0",


Please remove caret to pin the version.

AWolf81 · 2018-08-15T20:13:19Z

src/middlewares/task.middleware.js

+        // Child node processes will persist after their parent's death
+        // if they are not killed first, so we need to use `psTree` to build
+        // a tree of children and kill them that way.
+        psTree(processId, (err, children) => {


@superhawk610 Thanks for re-adding this and for adding the comment. I wasn't aware of this.
Can you please check main.js process killing and add child killing there as well?

The task killing during app run is working (tested with Win & Ubunutu). But the task killing doesn't work if the app is closed with a running dev server. PID still active - just tested with Ubuntu.

AWolf81 · 2018-08-15T20:17:48Z

src/services/platform.services.js

+    .replace('%USERPROFILE%\\', '')
+    .replace(/\s/g, '');
+}
+export const windowsHomeDir = isWin ? path.join(os.homedir(), winDocPath) : '';


Looks great. Thanks.

AWolf81 · 2018-08-15T20:32:03Z

src/main.js

-  );
-
-  return Promise.all(processKillingPromises).catch(err => {
+  try {


@superhawk610 Is this OK? I think the child killing first is needed here for Linux/Mac. Also it looks like there is no Windows version in killProccessId(processId).
What do you think should we add killProcess to platform.service? I think this would be useful so we can use it on app exit and for task killing on dev server close.

Great catch, I'm new to Electron so I just figured addProcessId and removeProcessId were internal Electron calls, I didn't realize we implemented them.

Yeah, we should probably move this to platform.service to avoid duplicating the implementation in main.js and task.middleware. I'll take this.

Moved it into services/kill-process-id.service.js and avoided ES6 modules syntax so it can be loaded both server- and client-side.

- consolidate import statements in task middleware - pin ps-tree version in package.json

AWolf81 · 2018-08-15T22:05:59Z

src/components/ApplicationMenu/ApplicationMenu.js

-            label: 'Import Existing Project',
+            label: __DARWIN__
+              ? 'Import Existing Project...'
+              : '&Import existing project...',
            click: showImportExistingProjectPrompt,
            accelerator: process.platform === 'darwin' ? 'Cmd+I' : 'Ctrl+I',


Nit - I think line 40 & 47 should also use the __DARWIN__ constant.

@mathieudutour actually already fixes this in #127, I figured there was no reason to add it here since we can just merge it with that PR.

ah, OK, you're right. I missed that.

- add disclaimer to kill-process-id service (avoid ES6)

- display Electron output in webpack console

React Rally is apparently a priority for people 😁 Seriously good luck with your talk!

- dont exit process on Electron errors

commit f01d78d Author: Mathieu Dutour <[email protected]> Date: Sat Aug 18 15:49:12 2018 -0500 defaultParentPath is a constant (#134) commit 37ae6c8 Author: Abhijeet Prasad <[email protected]> Date: Sat Aug 18 07:23:13 2018 -0700 Add tests for projects reducer (#84) * Add tests for projects reducer * Revert whitespace change to projects.reducer.js commit 425c7d4 Author: Mathieu Dutour <[email protected]> Date: Thu Aug 16 12:01:44 2018 -0500 ESLint config (#128) * add eslint config * add lint script and run eslint on precommit * fix lint errors commit 8bbb281 Author: Aaron Ross <[email protected]> Date: Thu Aug 16 12:59:07 2018 -0400 use correct package script for release build (#133) commit 9f0abef Author: Mathieu Dutour <[email protected]> Date: Thu Aug 16 11:58:32 2018 -0500 add menu item to report an issue (#127) commit efc6f57 Author: Alexander Wolf <[email protected]> Date: Thu Aug 16 18:33:52 2018 +0200 Windows support (#125) - `.cmd` formatting for Windows - modify `findAvailablePort` to work on all platforms - use `%HOME%/Documents/guppy-projects` for default Windows projects directory - changed package manager from NPM to yarn - add `cross-env` to allow package scripts to work on all platforms - move app-wide settings into `src/config/app.js` - change spawn process to use promises - stub `electron.remote` for headless testing - add `yarn` as local dependency to allow usage without global installation - forward host environment to Electron process - properly kill all spawned processes on app quit, cross-platform - exit main process when Electron application quits during development commit 566a392 Author: Joshua Comeau <[email protected]> Date: Wed Aug 15 15:02:21 2018 -0400 Remove unused components (#132) commit f61802e Author: Mathieu Dutour <[email protected]> Date: Tue Aug 14 06:30:25 2018 -0600 add react-devtools when developping (#123) * add react-devtools when developping * pin the dependencies * add comment about what is the id * load chrome extension on prod * move dependencies so that they are bundled in the app commit 71435a7 Author: Mathieu Dutour <[email protected]> Date: Mon Aug 13 07:30:02 2018 -0600 setup proper target for webpack and use import everywhere (#126) * setup proper target for webpack and use import * fix externals config commit cae946e Author: Josh Comeau <[email protected]> Date: Sun Aug 12 11:54:22 2018 -0400 Fix CircularOutline gradient on non-Chrome browsers commit 9831349 Author: Melanie Seltzer <[email protected]> Date: Fri Aug 3 05:25:38 2018 -0700 Dependencies explanation doc (#116) * Add understanding dependency markdown doc * Move HelpButton to own file for re-use, link out to new dependency info doc * Package.json info doc first pass * Update package.json doc per PR review commit 2c4120b Author: Joshua Comeau <[email protected]> Date: Thu Aug 2 08:55:55 2018 -0400 Add 'reason' to PixelShifter (#118)

bennygenel and others added 23 commits July 16, 2018 13:36

Initial changes

edfc88d

removed console.log and added prettier-ignore

df57950

fixes typo and missing isWin control, adds platform.services

c63ecc2

temporary solution for the eject error

e8ed3f3

added missing declarations on platform.services

eca70f0

Initial changes

34c0f50

removed console.log and added prettier-ignore

ecd609c

fixes typo and missing isWin control, adds platform.services

93e118c

temporary solution for the eject error

f5f6dfb

added missing declarations on platform.services

07a391d

redo task start commands

8595954

Merge branch 'windows-support' of https://github.com/bennygenel/guppy …

bfc7284

…into windows-support modified: scripts/start.js modified: src/middlewares/task.middleware.js

changed to yarn using package-manager.service

765fc31

used platform.service & re-structured PACKAGE_MANAGERS constant

4c48f6e

using PACKAGE_MANAGER_CMD from platform.service

3db32e5

used spawnProcess from logger branch.

05821bf

added app config

88ba688

removed package-manager.service

e7eb1fa

added yarn locally to Guppy.

d5db626

simplified spawnProcess

8671d6c

merged bennygenel/windows-support

86c83e4

Merge remote-tracking branch 'fork-awolf/npm-to-yarn' into windows-su…

3cd9ac7

…pport

merged awolf81/npm-to-yarn

4715691

AWolf81 mentioned this pull request Aug 11, 2018

Npm to yarn bennygenel/guppy#3

Closed

joshwcomeau reviewed Aug 12, 2018

View reviewed changes

fixed isWin function call.

9c34466

AWolf81 commented Aug 12, 2018

View reviewed changes

Removed unused imports

6b84e13

superhawk610 added 3 commits August 15, 2018 14:02

cache platform-specific function calls

1916572

kill orphaned processes on Mac

d832f7e

remove whitespace from gatsby dev server args

4f877d5

superhawk610 force-pushed the windows-support branch from 0642c0a to 4f877d5 Compare August 15, 2018 18:35

remove top-level Windows-only code

6ee5c36

superhawk610 approved these changes Aug 15, 2018

View reviewed changes

mathieudutour approved these changes Aug 15, 2018

View reviewed changes

AWolf81 commented Aug 15, 2018

View reviewed changes

superhawk610 added 3 commits August 15, 2018 16:49

rename services/platform.service.js

e9e1bee

- consolidate import statements in task middleware - pin ps-tree version in package.json

kill child processes on app exit correctly cross-platform

73d5421

conform menu item capitalization to os standards

00d0220

AWolf81 commented Aug 15, 2018

View reviewed changes

superhawk610 added 3 commits August 15, 2018 18:46

remove ipcRenderer call from kill-process-id service

7140bf1

- add disclaimer to kill-process-id service (avoid ES6)

quit webpack server when Electron quits

0cb271c

- display Electron output in webpack console

Merge branch 'master' into windows-support

adffe91

superhawk610 mentioned this pull request Aug 15, 2018

ESLint config #128

Merged

4 tasks

add missing declaration

e5c0b0e

superhawk610 added 2 commits August 15, 2018 19:37

update README and CONTRIBUTING for cross-platform

7b05826

- dont exit process on Electron errors

bump minor version

2dba6a3

superhawk610 merged commit efc6f57 into master Aug 16, 2018

superhawk610 deleted the windows-support branch August 16, 2018 16:33

superhawk610 mentioned this pull request Aug 16, 2018

[WIP] Windows Support #63

Closed

superhawk610 mentioned this pull request Sep 9, 2018

WIP: #26 Move to electron-builder #236

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows support #125

Windows support #125

AWolf81 commented Aug 11, 2018 •

edited

Loading

joshwcomeau left a comment

joshwcomeau commented Aug 12, 2018

AWolf81 Aug 12, 2018

AWolf81 Aug 12, 2018

AWolf81 commented Aug 12, 2018 •

edited

Loading

superhawk610 left a comment

mathieudutour left a comment

superhawk610 commented Aug 15, 2018

mathieudutour commented Aug 15, 2018

superhawk610 commented Aug 15, 2018 •

edited

Loading

mathieudutour commented Aug 15, 2018

superhawk610 commented Aug 15, 2018 •

edited

Loading

AWolf81 Aug 15, 2018

superhawk610 Aug 15, 2018

AWolf81 Aug 15, 2018

AWolf81 Aug 15, 2018

AWolf81 Aug 15, 2018

AWolf81 Aug 15, 2018

superhawk610 Aug 15, 2018

superhawk610 Aug 15, 2018 •

edited

Loading

AWolf81 Aug 15, 2018

superhawk610 Aug 15, 2018

AWolf81 Aug 15, 2018

Windows support #125

Windows support #125

Conversation

AWolf81 commented Aug 11, 2018 • edited Loading

joshwcomeau left a comment

Choose a reason for hiding this comment

joshwcomeau commented Aug 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AWolf81 commented Aug 12, 2018 • edited Loading

superhawk610 left a comment

Choose a reason for hiding this comment

mathieudutour left a comment

Choose a reason for hiding this comment

superhawk610 commented Aug 15, 2018

mathieudutour commented Aug 15, 2018

superhawk610 commented Aug 15, 2018 • edited Loading

mathieudutour commented Aug 15, 2018

superhawk610 commented Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superhawk610 Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AWolf81 commented Aug 11, 2018 •

edited

Loading

AWolf81 commented Aug 12, 2018 •

edited

Loading

superhawk610 commented Aug 15, 2018 •

edited

Loading

superhawk610 commented Aug 15, 2018 •

edited

Loading

superhawk610 Aug 15, 2018 •

edited

Loading