Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

NodeDomain#6193

Merged
jasonsanjose merged 5 commits intomasterfrom
iwehrman/node-domain
Dec 18, 2013
Merged

NodeDomain#6193
jasonsanjose merged 5 commits intomasterfrom
iwehrman/node-domain

Conversation

@iwehrman
Copy link
Copy Markdown
Contributor

@iwehrman iwehrman commented Dec 6, 2013

This pull request has three parts:

  1. A new NodeDomain module, which is a high-level wrapper around NodeConnection. This makes it easy to open a connection and load a single domain, and then execute the domain's commands and listen for its events. The wrapper handles, in particular, connection and domain loading and reloading.
    The inline documentation gives the following usage example:
var myDomain = new NodeDomain("someDomain", "/path/to/SomeDomainDef");
      $result = myDomain.exec("someCommand", arg1, arg2);

      $result.done(function (value) {
        // the command succeeded!
      });

      $result.fail(function (err) {
        // the command failed; act accordingly!
      });

      $(myDomain).on("someEvent", someHandler);
  1. A refactoring of StaticServer to use NodeDomain instead of manipulating NodeConnection directly. The previous code had been copied and pasted regularly, but had a few shortcomings, including redundant timeouts (NodeConnection handles this already), failure to handle disconnected sockets, and, in some cases, a silent fail-fast behavior on command execution where simply waiting would suffice. (If the NodeConnection isn't connected, don't abort; just wait for it to come back up!)
  2. Finally, it also fixes a race condition in StaticServerDomain that was causing the LiveDevelopment unit tests to fail unpredictably.
    The problem is as follows: when StaticServerDomain receives an HTTP request for a filtered path, it emits a filterRequest event to Brackets to have the file contents at that path manipulated before sending the response. Brackets later calls the writeFilteredResponse command with the manipulated data, which triggers the HTTP response. StaticServerDomain connects these filtered responses to their original filter requests by way of a map of callbacks keyed on the requested paths. But, as a consequence, when the StaticServerDomain HTTP server receives two concurrent requests for the same path, it only stores one response callback. The second filtered response is then be dropped, no longer having any response callback at the given path, and the second request---whose response callback has been overwritten---will always time out. The problem is solved here by adding a unique identifier to each filterRequest and keying callbacks on this identifier.

@ghost ghost assigned jasonsanjose Dec 6, 2013
@jasonsanjose
Copy link
Copy Markdown
Member

Oops. Accidentally closed.

@jasonsanjose jasonsanjose reopened this Dec 6, 2013
@ghost ghost assigned jasonsanjose Dec 6, 2013
@jasonsanjose
Copy link
Copy Markdown
Member

Assigned to me

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right? Looking at the usage it should be {Object.<string, {Object.<number, function>}} and A map from root paths to its request to reponse callback mapping.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er, right, good catch. I'll fix that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you make this change? I don't see it.

@jasonsanjose
Copy link
Copy Markdown
Member

Interesting. I just happened to have an open live preview session in Brackets when I ran the StaticServer unit tests. I saw about half of them fail. However, the tests did pass after I closed the session.

@jasonsanjose
Copy link
Copy Markdown
Member

Was there any reason besides time to not update the other usages of NodeConnection? Just curious if there were other gotchas that would prohibit use.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 promise() method...

@jasonsanjose
Copy link
Copy Markdown
Member

Initial review complete @iwehrman. Nice work.

@iwehrman
Copy link
Copy Markdown
Contributor Author

iwehrman commented Dec 6, 2013

The only reason I didn't convert more NodeConnection clients to NodeDomain was time. Also I didn't think you'd merge it unless I refactored at least one client :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants