Extract the starting functionality into an exported class.

[JoshWillik]

closes #3789

• Create a GhostServer class to manage state
• index.js now calls start on exported state instance
• Alter tests to expect an instance of GhostServer instead of an express app

[ErisDS]

This is pretty damned awesome 🎈 unfortunately I merged another big refactoring PR that got in first, and it has caused a conflict. If you have a minute to rebase it would be much appreciated.

[JoshWillik]

I can't get to it right away, but I'll look at getting that sorted out
tonight.
Work for you?

On Tue, Aug 19, 2014 at 10:30 AM, Hannah Wolfe notifications@github.com
wrote:

This is pretty damned awesome [image: 🎈] unfortunately I merged
another big refactoring PR that got in first, and it has caused a conflict.
If you have a minute to rebase it would be much appreciated.

—
Reply to this email directly or view it on GitHub
#3815 (comment).

[ErisDS]

Oh aye no rush :)

[ErisDS]

Ooh, looks like you used merge rather than rebase? Rebasing should still leave you with just a single commit on the PR rather than collecting up other bits and pieces. No worries as I should be able to fix it up, but I recommend using rebase for updating PRs from master.

[JoshWillik]

I'm pretty sure I did a rebase initially, but git wouldn't let me --continue, and I ended up running random commands until I coerced the codebase into a state that was correct. :P
If you'd like, I can roll my code and give it another crack

[ErisDS]

@JoshWillik your call, it's your PR :) I'm happy to take a look if it's gotten too messy. Top tip: no matter what, you can always update the PR by pushing a branch with the same name as the original branch the PR was made from ;)

[JoshWillik]

Not totally sure if I'm reading this correctly.
You're saying that I can update the extract-init PR by pushing new commits to the extract-init branch?
Does the same apply if I roll back and retry the rebasing and push with a -f?

[ErisDS]

Indeed! In fact if it all goes totally wonk, you can make a make a new branch, cherry pick, delete the old branch, rename your new branch to have the same name and push with a -f and this PR will pick that up! I only mention it because people often assume they've 'broken' the PR and close it when that's not necessary :)

[JoshWillik]

Ah excellent. It's good to know that PRs are so versatile.
I'll try to patch this up tonight and push a fixed version

—
Sent from Mailbox

On Wed, Aug 20, 2014 at 3:31 PM, Hannah Wolfe notifications@github.com
wrote:

Indeed! In fact if it all goes totally wonk, you can make a make a new branch, cherry pick, delete the old branch, rename your new branch to have the same name and push with a -f and this PR will pick that up! I only mention it because people often assume they've 'broken' the PR and close it when that's not necessary :)

Reply to this email directly or view it on GitHub:
#3815 (comment)

[hswolff]

this is rawsome

[JoshWillik]

I'm fairly sure it's sorted out at this point. My apologies for all the nonsense leading up to this point.

[ErisDS]

Beautiful 💃

[JoshWillik]

Once this is merged, I plan to continue teasing the internals of Ghost out into something that can be used as an express module.
Do you have any timeframe for the merge? :)

[ErisDS]

Am just testing it out ;)

[JoshWillik]

Aha fair enough :P
I'll be patient

[ErisDS]

I am super excited about this change, just took it for a spin and all is well.

There is one concern, and that this is a very breaking change for anyone using Ghost as an npm module. I really don't want to have to hold this back for a major release or something lame like that - I want to hit the merge button right now!

So, the question I wanted to throw out there is how best to message that this change will occur with 0.5.2? Ideally, I'd like the message to only appear to people using Ghost via npm, otherwise it'll confuse people.

I thought about echoing a warning from one of the npm scripts but I can't see a hook that would fire at the right time. The other thought I had was to add a message which displays if ghost() is passed an object (which the index.js file in Ghost doesn't do). I think most people using ghost as an npm module are passing something there, but we'll certainly miss a few?

Final idea was to put a message somewhere in the ghost() call with a timeout that gets cancelled if app.start() gets called within 10 seconds of ghost() - so the message only gets displayed if you call ghost() and don't call app.start() quickly after.

I can obviously also put a big warning on the releases page.

Just wondering if anyone can think of a better way?

@JoshWillik I'm not asking you to do this as part of this PR. My finger is hovering over the merge button and as soon as I'm sure we have an acceptable plan for how to message this I'll hit it!

[ErisDS]

Update: Whoops sorry, I just did more testing and realised that app.stop() isn't actually stopping the server. It still serves requests as though nothing has happened.

[ErisDS]

I tested out changing these two lines and that fixes .stop(). Sorry in my excitement I really have gone about this a bit backwards.

[JoshWillik]

@ErisDS I'm not sure how this is a breaking change for those using NPM.

If you require( 'ghost' ), ghost auto-starts as before.
It's only when you require( 'ghost/core' ) that you have the option of not starting the server up, no?

If, however, a notification is required, it shouldn't be too hard to do something like this:

function GhostServer(app) {
    this.app = app;
    this.httpListener = null;
    this.upgradeTimeout = setTimeout( this.upgradeNotes.bind( this ), 10000 ) //HERE
}

GhostServer.prototype.start = function () {
    // ...starting code
    this.httpServer.on('listening', function () {
        this.logStartMessages();
        clearTimeout( this.upgradeTimeout ) //AND HERE
        deferred.resolve(this);
    }.bind(this));
    return deferred.promise;
}

GhostServer.prototype.upgradeNotes = function(){
    console.log( 'We have upgraded, notes notes' )
}

This change adds very little complexity into the startup process, and is easy to remove when it's no longer needed.
Sound good?

[JoshWillik]

Update

Sorry, it seems I failed to look at the package.json, core/index is invoked directly on npm start instead of index, as I had assumed.
As a followup idea then, perhaps the package.json could be modified to call index as though it were run from the command line?

[ErisDS]

Aye that's why it's such a big deal ;) I don't want to change main to call the root index.js though as this would prevent access to your shiny new API for starting and stopping Ghost.

I think the upgradeTimeout message thing is the best way to resolve this problem. We're taking a leap forward towards being middleware, which I think the people using the npm module will very much appreciate. So I don't think having to change their code is an issue - as long as we message it, as otherwise their Ghost will just hang and not do anything and they'll not know why!

[JoshWillik]

Okay cool. I'll batch this change in with the fixes to the httpServer variable.
Do you have any specific message text that you wanted in the upgradeTimeout? Otherwise I'll just improvise.

[ErisDS]

Message something like:

Warning: Ghost will no longer start automatically when using it as an npm module. Please see the docs for information on how to update your code.

[hswolff]

So just to be clear the only change npm users will have to do is,

Instead of doing this:

var ghost = require('ghost');
ghost();

They will now have to do this:

var ghost = require('ghost');
ghost().start();

Right?

[ErisDS]

@hswolff I've already updated the docs ;)

The API is this:

var ghost = require('ghost');
ghost().then(function (app) {
    app.start();
});

[JoshWillik]

@ErisDS 👍

[hswolff]

So as far as middleware goes...that's not what express middleware currently looks like.

Typically you get immediately returned a function that you can then attach to your express instance.

I'm not sure we're able to synchronously return a function at this point in time, however we should in the future strive to return a function immediately that can then be mounted.

Just some food for thought for the future. If we can reduce the amount of times we message to users how usage has changed the easier for everyone.

[JoshWillik]

Since the ghost internals rely so heavily on promises, I don't see an easy way to make the initial require return a synchronous result.

Despite this, mounting a ghost blog could still work:

var app = require( 'express' )()
var ghost = require( 'ghost' )
ghost().then( function( blog ) {
  app.use( '/blog', blog.app )
  app.listen( 8888 )
})

It doesn't follow the normal express pattern perfectly, but I think it's workable enough for those of us who want a blog mounted in an express app.

[JoshWillik]

Update

@hswolff on second thought, it might be possible to create another entry point that has a footprint like so:

var app = require( 'express' )()
var middleware = require( 'ghost/middleware' )
app.use( '/blog', middleware() )

I'm 95% confident that I can make a middleware.js file that would act like regular express middleware and just buffer the requests until ghost starts up.

Does this sound good to you? It's more complex than the average express middleware, but it's certainly simpler than the code snippet I demo'd in the last comment.

[ErisDS]

Whilst obviously, the end goal here is to achieve the ability to mount ghost as express middleware, and your suggestion sounds great, I don't think that detracts from the value of this PR?

As in, we should get this patched and merged, and then we have a new API for controlling Ghost - the start and stopping of the server is isolated from the bootup code, and this will be useful for some advanced use cases (like people who want to add basic auth to the front of their blog). I think it will also make the code more easily testable.

Then you can follow up with a second PR adding the middleware file which makes Ghost work as express middleware, which is like.. a whole other level of awesome?

[JoshWillik]

@ErisDS That sounds like a solid plan.
I'll patch up the issues mentioned above, get that submitted, then I'll start working away at middleware.

[ErisDS]

👍 If it's done by ~2nd September, it'll all go out in an official release together, and ghost-as-npm-module users will be presented with the most excellent of choices.

[JoshWillik]

I see no reason I can't get this done by then.
I hope to have it running by Sunday, actually, but no promises there.

Update: Hah, promises? Because ghost uses... pr... forget it, bad joke

[JoshWillik]

It seems that the upgrade warning makes the Travis build logs really confusing. It still passes though.

[ErisDS]

Confusing travis logs aren't the end of the world, we could disable this message when running in the testing environment, but it also acts as a reminder to remove the warning message at some point :)

[JoshWillik]

Good call :)

—
Sent from Mailbox

On Fri, Aug 22, 2014 at 2:39 PM, Hannah Wolfe notifications@github.com
wrote:

Confusing travis logs aren't the end of the world, we could disable this message when running in the testing environment, but it also acts as a reminder to remove the warning message at some point :)

Reply to this email directly or view it on GitHub:
#3815 (comment)