Get demo working on Windows #130
Conversation
- Removed bin bash scripts in favor of npm scripts so they work cross-platform - Moved to use 127.0.0.1 as 0.0.0.0 doesn't work on Windows Note that this doesn't work with the current pty.js as it doesn't build on Windows.
|
Putting this on hold, until some important issues are resolved: #132. |
|
Merged in master and resolved conflicts, this is unblocked now |
| @@ -46,7 +46,7 @@ app.ws('/bash', function(ws, req) { | |||
| }); | |||
|
|
|||
| var port = process.env.PORT || 3000, | |||
| host = '0.0.0.0'; | |||
There was a problem hiding this comment.
Should we keep this to 0.0.0.0 to make sure this works for VMs/remote servers?
There was a problem hiding this comment.
Windows would let me go to 127.0.0.1 in a browser when host="0.0.0.0", it just wouldn't let me go to 0.0.0.0. I mainly just wanted the message and readme to work without issues on Windows for this.
There was a problem hiding this comment.
What about detecting platform and setting this to 127.0.0.1 if on windows?
There was a problem hiding this comment.
Doable, ideally we would only have a single URL in the readme though.
There was a problem hiding this comment.
Yes, that's why I left README intact. In order to give more examples of what I mean, if we run this inside a Docker container it won't be accessible.
That's why I prefer the 0.0.0.0 bind. If we detect the platform though it would be fine to listen to localhost IMO, Windows systems will work fine in localhost, since it's most probably a personal computer.
There was a problem hiding this comment.
Updated, let me know if this is what you had in mind.
|
LGTM, just a minor comment and I believe we can merge this. Thanks @Tyriar ! |
cross-platform
Note that this doesn't work with the current pty.js as it doesn't build on
Windows.
The last step in getting it working for Windows was changing pty.js to point at https://github.com/Tyriar/pty.js/tarball/prebuilt - this may break compatibility with older versions of node though, I'm not certain.