User can now specify a port for the live dev server #6815
Conversation
| @@ -102,7 +104,7 @@ define(function (require, exports, module) { | |||
| */ | |||
| StaticServer.prototype.readyToServe = function () { | |||
| var self = this; | |||
| return this._nodeDomain.exec("getServer", self._root) | |||
| return this._nodeDomain.exec("getServer", self._root, self._port) | |||
There was a problem hiding this comment.
I think this is where the PreferencesManager.get call should be. That way, the port is read at the time you start Live Development (giving the user a chance to set the value, rather than requiring setting the value and then restarting Brackets).
| * @param {number} port number to clean, can be string as well | ||
| * @return {number} a valid port number or zero if a value wasn't passed in or valid. | ||
| */ | ||
| function sanatizePort(port) { |
There was a problem hiding this comment.
Nit: it should be sanitizePort.
|
Your change looks good overall (great to see all of the comments and tests!) I'll do some testing after you've changed where the port preference is looked up. Thanks! |
|
Ooops. Didn't mean to close the request. |
|
I made the suggested changes (and fixed that idiotic spelling mistake). I had to do a little refactoring in the The changes were 1) to switch pref to "staticserver.port", 2) move pref loading to last second before requesting server, and 3) fix a spelling mistake. |
| * @param {number} port number to clean, can be string as well | ||
| * @return {number} a valid port number or zero if a value wasn't passed in or valid. | ||
| */ | ||
| function sanitizePort(port) { |
There was a problem hiding this comment.
It seems like it's probably safe to just do this on one side of the connection. You're now doing this on the client side. Or is there a code path I haven't noticed yet that could still pass an unsanitized port across?
There was a problem hiding this comment.
I vacillated on that and just decided to have them both in, but I agree
that it's probably safe to not duplicate it. I wanted it in StaticServer
to clean up any invalid pref values which was necessary for detecting port
changes. I would get rid of it on the StaticServerDomain side since
currently there is nothing else calling it.
On Thu, Feb 13, 2014 at 3:16 PM, Kevin Dangoor notifications@github.comwrote:
In src/extensions/default/StaticServer/node/StaticServerDomain.js:
@@ -98,7 +98,20 @@
function normalizeRootPath(path) {
return (path && path[path.length - 1] === "/") ? path.slice(0, -1) : path;}
- /**
\* @private\* Converts given value to a valid port number or zero.\* A zero will cause a random port to be used.\* @param {number} port number to clean, can be string as well\* @return {number} a valid port number or zero if a value wasn't passed in or valid.*/- function sanitizePort(port) {
It seems like it's probably safe to just do this on one side of the
connection. You're now doing this on the client side. Or is there a code
path I haven't noticed yet that could still pass an unsanitized port across?Reply to this email directly or view it on GitHubhttps://github.com//pull/6815/files#r9727721
.
There was a problem hiding this comment.
OK, I can do that easily when I'm testing before I merge.
|
Merged. Thanks for the improvement! |
Overview
These changes allow a user to specify a fixed port number, via a preference, for the Live Development server. This fixes the the now-closed Issue 5626.
Use Cases
I was struggling with the changing of port numbers while developing an application which used IndexedDB as well as the Google Drive API.
IndexedDB databases are host and port specific, thus any development data I stored while using one port number was lost each time I closed and reopened Brackets.
With the Google Drive API, applications must be served from hosts (and port) that the developer has registered through the API's console, thus I needed to be log into the Google API Console and update it each time Brackets was closed and reopened.
Solution
The StaticServer extension was the only area modified in order to support a user-specified port. StaticServer is Bracket's internal HTTP server, and is used solely by the LiveDevelopment module. Three sets of changes were necessary:
StaticServerDomain.js
StaticServerDomain, the actual server code executed in the node process, was updated to accept a port number in it's
getServercommand. If the passed in port isn't a valid port number or if the port number is already in use, StaticServerDomain will fallback to it's original behavior of serving from a random port. It may be desirable to log a warning when this happens, but I didn't code that.The "listening" callback used when calling server.listen was factored out to a long-term listener now added via
server.on("listening", fn). This accommodates a second call to server.listen that appears in theserver.on("error", fn)which could get called in the event that the specified port is already in use.It's important to note that the public API for getServer has changed from
getServer(host, cb)togetServer(host, port, cb). There was only one place in core that needed updating for this as well as the unit tests, but this would break anyone else's extensions that explicitely call getServer.StaticServer.js
StaticServer is the Bracket-side wrapper around the node domain. It was changed to accept a port number in the config object that is passed to it's constructor. StaticServer doesn't do anything with the port number other than pass it along to the node domain when calling getServer.
main.js
StaticServer's main.js pulls the user-specified port from preferences using the key "liveDevPort" and passes that value into the StaticServer constructor.
Tests
I added two tests. One tests that the specified port is used when it's passed in. The other tests that a random port is used if the specified port is already in use. The default case, when a port isn't specified by the user, is tested explicitly in an existing test.