-
Notifications
You must be signed in to change notification settings - Fork 7.5k
NodeDomain #6193
NodeDomain #6193
Changes from 2 commits
8349821
e320244
93b1187
5adeb59
434b0db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,10 +43,10 @@ define(function (require, exports, module) { | |
| * baseUrl - Optional base URL (populated by the current project) | ||
| * pathResolver - Function to covert absolute native paths to project relative paths | ||
| * root - Native path to the project root (and base URL) | ||
| * nodeConnection - An active NodeConnection | ||
| * nodeDomain - An initialized NodeDomain | ||
| */ | ||
| function StaticServer(config) { | ||
| this._nodeConnection = config.nodeConnection; | ||
| this._nodeDomain = config.nodeDomain; | ||
| this._onRequestFilter = this._onRequestFilter.bind(this); | ||
|
|
||
| BaseServer.call(this, config); | ||
|
|
@@ -61,7 +61,7 @@ define(function (require, exports, module) { | |
| * @return {boolean} true for yes, otherwise false. | ||
| */ | ||
| StaticServer.prototype.canServe = function (localPath) { | ||
| if (!this._nodeConnection.connected()) { | ||
| if (!this._nodeDomain.ready()) { | ||
| return false; | ||
| } | ||
|
|
||
|
|
@@ -87,17 +87,9 @@ define(function (require, exports, module) { | |
| * @return {jQuery.Promise} Resolved by the StaticServer domain when the message is acknowledged. | ||
| */ | ||
| StaticServer.prototype._updateRequestFilterPaths = function () { | ||
| if (!this._nodeConnection.connected()) { | ||
| return; | ||
| } | ||
|
|
||
| var paths = []; | ||
|
|
||
| Object.keys(this._liveDocuments).forEach(function (path) { | ||
| paths.push(path); | ||
| }); | ||
| var paths = Object.keys(this._liveDocuments); | ||
|
|
||
| return this._nodeConnection.domains.staticServer.setRequestFilterPaths(this._root, paths); | ||
| return this._nodeDomain.exec("setRequestFilterPaths", this._root, paths); | ||
| }; | ||
|
|
||
| /** | ||
|
|
@@ -109,37 +101,14 @@ define(function (require, exports, module) { | |
| * the server is ready/failed. | ||
| */ | ||
| StaticServer.prototype.readyToServe = function () { | ||
| var readyToServeDeferred = $.Deferred(), | ||
| self = this; | ||
|
|
||
| if (this._nodeConnection.connected()) { | ||
| this._nodeConnection.domains.staticServer.getServer(self._root).done(function (address) { | ||
| var self = this; | ||
| return this._nodeDomain.exec("getServer", self._root) | ||
| .done(function (address) { | ||
| self._baseUrl = "http://" + address.address + ":" + address.port + "/"; | ||
| readyToServeDeferred.resolve(); | ||
| }).fail(function () { | ||
| }) | ||
| .fail(function () { | ||
| self._baseUrl = ""; | ||
| readyToServeDeferred.reject(); | ||
| }); | ||
| } else { | ||
| // nodeConnection has been connected once (because the deferred | ||
| // resolved, but is not currently connected). | ||
| // | ||
| // If we are in this case, then the node process has crashed | ||
| // and is in the process of restarting. Once that happens, the | ||
| // node connection will automatically reconnect and reload the | ||
| // domain. Unfortunately, we don't have any promise to wait on | ||
| // to know when that happens. The best we can do is reject this | ||
| // readyToServe so that the user gets an error message to try | ||
| // again later. | ||
| // | ||
| // The user will get the error immediately in this state, and | ||
| // the new node process should start up in a matter of seconds | ||
| // (assuming there isn't a more widespread error). So, asking | ||
| // them to retry in a second is reasonable. | ||
| readyToServeDeferred.reject(); | ||
| } | ||
|
|
||
| return readyToServeDeferred.promise(); | ||
| }; | ||
|
|
||
| /** | ||
|
|
@@ -181,26 +150,27 @@ define(function (require, exports, module) { | |
| * Send HTTP response data back to the StaticServerSomain | ||
| */ | ||
| StaticServer.prototype._send = function (location, response) { | ||
| if (this._nodeConnection.connected()) { | ||
| this._nodeConnection.domains.staticServer.writeFilteredResponse(location.root, location.pathname, response); | ||
| } | ||
| this._nodeDomain.exec("writeFilteredResponse", location.root, location.pathname, response); | ||
| }; | ||
|
|
||
| /** | ||
| * @private | ||
| * Event handler for StaticServerDomain requestFilter event | ||
| * @param {jQuery.Event} event | ||
| * @param {{hostname: string, pathname: string, port: number, root: string}} request | ||
| * @param {{hostname: string, pathname: string, port: number, root: string, id: number}} request | ||
| */ | ||
| StaticServer.prototype._onRequestFilter = function (event, request) { | ||
| var key = request.location.pathname, | ||
| liveDocument = this._liveDocuments[key], | ||
| response = null; | ||
|
|
||
| response; | ||
| // send instrumented response or null to fallback to static file | ||
| if (liveDocument && liveDocument.getResponseData) { | ||
| response = liveDocument.getResponseData(); | ||
| } else { | ||
| response = {}; | ||
| } | ||
| response.id = request.id; | ||
|
|
||
| this._send(request.location, response); | ||
| }; | ||
|
|
@@ -209,15 +179,15 @@ define(function (require, exports, module) { | |
| * See BaseServer#start. Starts listenting to StaticServerDomain events. | ||
| */ | ||
| StaticServer.prototype.start = function () { | ||
| $(this._nodeConnection).on("staticServer.requestFilter", this._onRequestFilter); | ||
| $(this._nodeDomain).on("requestFilter", this._onRequestFilter); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Much better. The domainevent notation is a bit confusing when you're used to namespaces. |
||
| }; | ||
|
|
||
| /** | ||
| * See BaseServer#stop. Remove event handlers from StaticServerDomain. | ||
| */ | ||
| StaticServer.prototype.stop = function () { | ||
| $(this._nodeConnection).off("staticServer.requestFilter", this._onRequestFilter); | ||
| $(this._nodeDomain).off("requestFilter", this._onRequestFilter); | ||
| }; | ||
|
|
||
| exports.StaticServer = StaticServer; | ||
| module.exports = StaticServer; | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,31 +34,15 @@ define(function (require, exports, module) { | |
| FileUtils = brackets.getModule("file/FileUtils"), | ||
| LiveDevServerManager = brackets.getModule("LiveDevelopment/LiveDevServerManager"), | ||
| BaseServer = brackets.getModule("LiveDevelopment/Servers/BaseServer").BaseServer, | ||
| NodeConnection = brackets.getModule("utils/NodeConnection"), | ||
| NodeDomain = brackets.getModule("utils/NodeDomain"), | ||
| ProjectManager = brackets.getModule("project/ProjectManager"), | ||
| StaticServer = require("StaticServer").StaticServer; | ||
|
|
||
| /** | ||
| * @const | ||
| * Amount of time to wait before automatically rejecting the connection | ||
| * deferred. If we hit this timeout, we'll never have a node connection | ||
| * for the static server in this run of Brackets. | ||
| */ | ||
| var NODE_CONNECTION_TIMEOUT = 5000; // 5 seconds | ||
|
|
||
| /** | ||
| * @private | ||
| * @type{jQuery.Deferred.<NodeConnection>} | ||
| * A deferred which is resolved with a NodeConnection or rejected if | ||
| * we are unable to connect to Node. | ||
| */ | ||
| var _nodeConnectionDeferred = new $.Deferred(); | ||
| StaticServer = require("StaticServer"); | ||
|
|
||
| /** | ||
| * @private | ||
| * @type {NodeConnection} | ||
| * @type {NodeDomain} | ||
| */ | ||
| var _nodeConnection = new NodeConnection(); | ||
| var _nodeDomain; | ||
|
|
||
| /** | ||
| * @private | ||
|
|
@@ -67,61 +51,26 @@ define(function (require, exports, module) { | |
| */ | ||
| function _createStaticServer() { | ||
| var config = { | ||
| nodeConnection : _nodeConnection, | ||
| nodeDomain : _nodeDomain, | ||
| pathResolver : ProjectManager.makeProjectRelativeIfPossible, | ||
| root : ProjectManager.getProjectRoot().fullPath | ||
| }; | ||
|
|
||
| return new StaticServer(config); | ||
| } | ||
|
|
||
| /** | ||
| * Allows access to the deferred that manages the node connection. This | ||
| * is *only* for unit tests. Messing with this not in testing will | ||
| * potentially break everything. | ||
| * | ||
| * @private | ||
| * @return {jQuery.Deferred} The deferred that manages the node connection | ||
| */ | ||
| function _getNodeConnectionDeferred() { | ||
| return _nodeConnectionDeferred; | ||
| } | ||
|
|
||
| function initExtension() { | ||
| // Start up the node connection, which is held in the | ||
| // _nodeConnectionDeferred module variable. (Use | ||
| // _nodeConnectionDeferred.done() to access it. | ||
| var connectionTimeout = setTimeout(function () { | ||
| console.error("[StaticServer] Timed out while trying to connect to node"); | ||
| _nodeConnectionDeferred.reject(); | ||
| }, NODE_CONNECTION_TIMEOUT); | ||
|
|
||
| _nodeConnection.connect(true).then(function () { | ||
| _nodeConnection.loadDomains( | ||
| [ExtensionUtils.getModulePath(module, "node/StaticServerDomain")], | ||
| true | ||
| ).then( | ||
| function () { | ||
| clearTimeout(connectionTimeout); | ||
|
|
||
| // Register as a Live Development server provider | ||
| LiveDevServerManager.registerServer({ create: _createStaticServer }, 5); | ||
| var modulePath = ExtensionUtils.getModulePath(module, "node/StaticServerDomain"); | ||
| _nodeDomain = new NodeDomain("staticServer", modulePath); | ||
|
|
||
| _nodeConnectionDeferred.resolveWith(null, [_nodeConnection]); | ||
| }, | ||
| function () { // Failed to connect | ||
| console.error("[StaticServer] Failed to connect to node", arguments); | ||
| _nodeConnectionDeferred.reject(); | ||
| } | ||
| ); | ||
| return _nodeDomain.promise().then(function () { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't seem necessary to wait for the promise here anymore. Is it?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe not? I wasn't quite sure, so I opted to try to preserve the existing functionality. Everything is so delicate with live development, so when in doubt I just tried not to shake anything. If you don't think it's necessary though I could also kill that |
||
| LiveDevServerManager.registerServer({ create: _createStaticServer }, 5); | ||
| return _nodeDomain.connection; | ||
| }); | ||
|
|
||
| return _nodeConnectionDeferred.promise(); | ||
| } | ||
|
|
||
| exports.initExtension = initExtension; | ||
|
|
||
| // For unit tests only | ||
| exports._getStaticServerProvider = _createStaticServer; | ||
| exports._getNodeConnectionDeferred = _getNodeConnectionDeferred; | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,13 @@ | |
| var _domainManager; | ||
|
|
||
| var FILTER_REQUEST_TIMEOUT = 5000; | ||
|
|
||
| /** | ||
| * @private | ||
| * @type {number} | ||
| * Used to assign unique identifiers to each filter request | ||
| */ | ||
| var _filterRequestCounter = 0; | ||
|
|
||
| /** | ||
| * @private | ||
|
|
@@ -68,8 +75,8 @@ | |
|
|
||
| /** | ||
| * @private | ||
| * @type {Object.<string, {Object.<string, http.ServerResponse>}} | ||
| * A map from root paths to its request/response mapping. | ||
| * @type {Object.<number, {Object.<string, http.ServerResponse>}} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this right? Looking at the usage it should be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Er, right, good catch. I'll fix that.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you make this change? I don't see it. |
||
| * A map from a request identifier to its request/response mapping. | ||
| */ | ||
| var _requests = {}; | ||
|
|
||
|
|
@@ -139,6 +146,7 @@ | |
| function rewrite(req, res, next) { | ||
| var location = {pathname: parse(req).pathname}, | ||
| hasListener = _rewritePaths[pathKey] && _rewritePaths[pathKey][location.pathname], | ||
| requestId = _filterRequestCounter++, | ||
| timeoutId; | ||
|
|
||
| // ignore most HTTP methods and files that we're not watching | ||
|
|
@@ -152,7 +160,7 @@ | |
| function resume(doNext) { | ||
| // delete the callback after it's used or we hit the timeout. | ||
| // if this path is requested again, a new callback is generated. | ||
| delete _requests[pathKey][location.pathname]; | ||
| delete _requests[pathKey][requestId]; | ||
|
|
||
| // pass request to next middleware | ||
| if (doNext) { | ||
|
|
@@ -163,12 +171,12 @@ | |
| } | ||
|
|
||
| // map request pathname to response callback | ||
| _requests[pathKey][location.pathname] = function (resData) { | ||
| _requests[pathKey][requestId] = function (resData) { | ||
| // clear timeout immediately when this callback is called | ||
| clearTimeout(timeoutId); | ||
|
|
||
| // response data is optional | ||
| if (resData) { | ||
| if (resData.body) { | ||
| // HTTP headers | ||
| var type = mime.lookup(location.pathname), | ||
| charset = mime.charsets.lookup(type); | ||
|
|
@@ -194,7 +202,8 @@ | |
|
|
||
| var request = { | ||
| headers: req.headers, | ||
| location: location | ||
| location: location, | ||
| id: requestId | ||
| }; | ||
|
|
||
| // dispatch request event | ||
|
|
@@ -308,11 +317,11 @@ | |
| * | ||
| * @param {string} path The absolute path of the server | ||
| * @param {string} root The relative path of the file beginning with a forward slash "/" | ||
| * @param {Object} resData Response data to use | ||
| * @param {!Object} resData Response data to use | ||
| */ | ||
| function _cmdWriteFilteredResponse(root, path, resData) { | ||
| var pathKey = getPathKey(root), | ||
| callback = _requests[pathKey][path]; | ||
| callback = _requests[pathKey][resData.id]; | ||
|
|
||
| if (callback) { | ||
| callback(resData); | ||
|
|
@@ -440,7 +449,7 @@ | |
| "requestFilter", | ||
| [{ | ||
| name: "location", | ||
| type: "{hostname: string, pathname: string, port: number, root: string}", | ||
| type: "{hostname: string, pathname: string, port: number, root: string: id: number}", | ||
| description: "request path" | ||
| }] | ||
| ); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually like the old way better where the command is a method on the domain. Is there a reason we can't do this with
NodeDomain?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that I wanted to make a wrapper that doesn't require you to wait on a promise before you start firing off commands. But we have to wait until the domain is loaded before we know what the commands are. If we do it this way, we can just wait for the connection to come up and the domains to be loaded before rejecting the exec promise if the command isn't there. In a previous version, I actually went ahead and bound all the loaded commands to the domain object itself at domain load time, but then if you tried to execute a command before the domain had been loaded you'd get a
TypeError. I also tried to find a way to override arbitrary property lookups for an object, but there doesn't seem to be a way to do this in ES5. I also would prefer to not have to pass strings around, but I personally think it's mostly just a style issue. (It's not like a static analysis could ever correctly predict the the properties on these domains anyway for the sake of, say, code hinting.)I only added the
promiseandreadymethods for backwards compatibility with the basic structure ofStaticServer. I don't consider them to really be core features ofNodeDomain.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points. Thanks for the clarification. I think you've made the right trade offs.