-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restart express server and trigger Livereload when a file under /server changes. #2265
Conversation
Yes, I think we will need to clear cache for all of
Most addons that I have seen, do not stash the express app instance, but just use it to add middlewares.
Shouldn't the debounce fix this?
Agreed. Easiest thing to do is to:
|
It's still only restarting the server once, but it's printing two change messages to the console:
|
The solution with the least problems is for this run in a managed child process |
I added a smoke test. It's demonstrating the failure condition. Tomorrow I will see about refactoring the http server out to run in a separate process. |
bda41a4
to
82b5b7d
Compare
Before going down the road of spawning a separate process to manage the express server, I tried invalidating the require cache for everything underneath server. It seems to have solved the corner cases I mentioned in my initial post, although I suppose some clever users might still be able to figure a way to break things. |
f6b57c1
to
3233036
Compare
@rwjblue So all of my tests are passing now, but somehow I broke some of the other smoke test. Any test that kills the |
f44f8f6
to
c46212e
Compare
Fixed the above problem. @rwjblue anything else you think this needs? |
c46212e
to
057ebab
Compare
Rebased to fix merge conflicts. |
057ebab
to
22ee045
Compare
I also think that a managed child process will make this more resilient. Is that something I can help you with, @csantero? |
@EndangeredMassa I looked into doing this as a managed process and realized that that turned this into a much larger project than I was willing to take on, mainly due to addon lifecycle issues. I chose instead to stomp the node cache for everything inside As of a month ago, I felt that the state of this PR looked generally good to merge, but was hoping to get some more feedback since it was a pretty big change. Unfortunately I have no time to work on this for the next several weeks, but if you want to build on my code you're welcome to fork my branch and submit a new PR. (BTW this PR does not yet support monitoring the Brocfile, which I agree would be a worthwhile feature.) |
if (!this.serverRestartScheduled) { | ||
this.serverRestartScheduled = true; | ||
var self = this; | ||
setTimeout(function () { |
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.
should we use: https://lodash.com/docs#throttle ?
@EndangeredMassa I would love to improve this effort to utilize child processes for a more resilient experience. That being said, I believe this looks good for a merge in the next day or so. But i would love if you had the energy to push continue the effort as @csantero is now busy. @csantero sorry for letting this linger, It honestly slipped past me. So this looks good, there are clearly some future improvements, any concerns to me merging it in now? |
I can make the handful of updates mentioned above. In doing so, I'll test it with one of our apps and get a feel for how well it works there. After that, a merge sounds good. Then maybe we could start a new issue to discuss the best way to approach a child process approach to this problem. |
alright, give me a 👍 when you feel good about it, also I am hoping to cut a new release tonight (if you happen to get it in by then it would be part of 0.1.3). Otherwise I'll cut next week again.
absolutely! |
I have the fixes locally, but can't test them because my global ember-cli refuses to run. I don't have time to address this tonight, but if you want to test it before merging and it works, then we can get it into this release. If not, I'll fix it and have it ready for the next release. Here's the PR. |
@EndangeredMassa @csantero hey guys, whats the status on this ? It appears the linked PR is still pending merge? |
@stefanpenner: @EndangeredMassa's change to use lodash's debounce broke some of the tests. I haven't had time to look into what happened. |
22ee045
to
32e46a4
Compare
@stefanpenner Ok we got that sorted out and Travis is happy. |
@csantero awesome, I will play with this on my machine after work (while at ember-nyc) and make sure it feels solid. If it's good, I'll include it in 0.1.3. Any concerns that I should hold off? Again thank you all for the work! |
@stefanpenner When I was last using this regularly in October (we've since moved away from using ember-cli's http mocks in our project for unrelated reasons), I think there were a few times the server watch functionality froze up and you'd have to restart |
@csantero awesome thanks One last question, is their a chance either of you can explore something similar for the brocfile? |
If @csantero doesn't have time, I'll add Brocfile support. |
Yeah thanks, @EndangeredMassa, if you make another PR to my fork I'll merge it. |
I will try this out as soon as I get to the meetup. Let's hope it goes well :) thanks friends! |
this feels great, just played with it in my apps for the last 30 or 40 minutes. Awesome work guys! |
Restart express server and trigger Livereload when a file under /server changes.
Huge thanks to @csantero @EndangeredMassa! |
Rationale
When working with http-mocks I get tired of having to tab to my terminal and restart
ember serve
every time I change a file.Summary of changes
This change introduces a development server restart mechanism. I created a new task called server-watcher, which wraps sane in a similar way to how watcher wraps broccoli-sane-watcher. The serve task creates an instance of server-watcher, and passes it to express-server. The express-server task is also now passed to the livereload-server task so it can be informed of server restarts.
Any file change/addition/deletion underneath the top-level
/server
directory will cause the following steps to be performed:Concerns
/server
, file A requires file B and then saves a copy of that exported module. If file B then changes, the logic in file A will still be depending on the old version of file B. The solution would be to delete the require cache for every file underneath/server
.add
and achange
event on file creation, and that's bubbling through to the UI as two separate messages.