search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] HTTP Server and Live Development #2788

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by redmunds Tuesday Feb 26, 2013 at 18:17 GMT Originally opened as https://github.com/adobe/brackets/pull/2955

@njx Code is mostly done, but there's a serious bug keeping Live Development integration tests from passing.

redmunds included the following code: https://github.com/adobe/brackets/pull/2955/commits

core-ai-bot commented 3 years ago

Comment by jasonsanjose Tuesday Feb 26, 2013 at 21:35 GMT

Need to merge with master again to kick off a new build on travis. JSHint dependencies were updated...see #2956.

core-ai-bot commented 3 years ago

Comment by njx Wednesday Feb 27, 2013 at 00:59 GMT

@joelrbrandt -- the updated node code (now named StaticServerDomain.js) is ready for you to take a look at. I made some comments around some of the changes besides the obvious ones.

Also, I chatted with@redmunds about making LiveDevelopment wait for the server startup promise to resolve--I think he's going to look at doing that.

I'm going to go ahead and review some of the rest of the code.

core-ai-bot commented 3 years ago

Comment by njx Wednesday Feb 27, 2013 at 01:56 GMT

Initial review done.

core-ai-bot commented 3 years ago

Comment by joelrbrandt Wednesday Feb 27, 2013 at 02:10 GMT

Reviewed@njx code in b9dcff7. One minor comment but otherwise looks great. Will comment on the feedback from@njx now and also look into the unit test failure.

core-ai-bot commented 3 years ago

Comment by joelrbrandt Wednesday Feb 27, 2013 at 02:35 GMT

@njx@redmunds Responses to initial review (that relate to me) are done. I've made some minor updates to code comments in 35e867f. There are still comments from@njx to@redmunds that need to be addressed.

I'm going to work on adding the wait-for-server-to-be-running promise now.

core-ai-bot commented 3 years ago

Comment by joelrbrandt Wednesday Feb 27, 2013 at 03:33 GMT

@redmunds@njx In c04c97e I implemented a readyToServe function which gets called at live development launch time and returns either a boolean or a promise.

This seems to fix all of the live development unit tests for me except for one: "should translate urls to/from local paths". From the errors, it looks like this is a unit test that is not yet "non-file:/// url" aware.

I've been running the tests on Mac. I haven't had time to test on windows yet. I'm cautiously optimistic that this fixes the unit test problems associated with server startup.

In an unrelated note, I have no idea what is going on with Travis. For awhile it said my new commit passed. Now it says it failed. The errors it is spitting out are crazy and have nothing to do with this code.@jasonsanjose any thoughts on Travis?

I'm wrapping up (on this) for the night. I'll be on for awhile working on other things. Ping me if you have questions.

core-ai-bot commented 3 years ago

Comment by joelrbrandt Wednesday Feb 27, 2013 at 03:35 GMT

Unfortunately, as part of my changes, a lot of the comments got marked as "outdated diffs". :-( Make sure you still look at them@redmunds -- there are unaddressed comments from@njx

core-ai-bot commented 3 years ago

Comment by redmunds Wednesday Feb 27, 2013 at 05:32 GMT

I think I responded to all remaining comments. Let us know if anything slipped through the cracks.

core-ai-bot commented 3 years ago

Comment by njx Wednesday Feb 27, 2013 at 19:25 GMT

Re-review done, and I pushed up fixes for@joelrbrandt's comments on my previous commits.@redmunds -- some of my non-nitpicky comments were in Joel's code, but you might want to take them over just so we can push this forward if he's busy today.

Also, looks like we need to merge from master.