Skip to content
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

Move all middleware into middleware module. #1372

Merged
merged 1 commit into from
Nov 11, 2013

Conversation

hswolff
Copy link
Contributor

@hswolff hswolff commented Nov 1, 2013

This PR moves all middleware functions that were previously in core/server.js into core/server/middleware.js.

This was done to improve the readability of core/server.js and to centralize all middleware functionality to one module.

Aside from moving the functions, they were modified to use the express application instance attached to the ghost singleton: ghost.server.

@ErisDS
Copy link
Member

ErisDS commented Nov 1, 2013

The main reason for moving middleware to middleware.js was to make it possible to write unit tests for them. I'd rather only move each function as and when it gets unit tests - unless you're happy to add them?

@hswolff
Copy link
Contributor Author

hswolff commented Nov 1, 2013

Happy to do so! Will get on it as soon as possible.

@hswolff
Copy link
Contributor Author

hswolff commented Nov 5, 2013

I've started working on creating tests for every middleware that I'm moving. All are going well except I've hit a snag and I'm not sure how to test middleware's that use the api module directly.

For example the redirectToSignup

function redirectToSignup(req, res, next) {
. Any help on how to best add a test case for that middleware?

I'm halfway through the middleware, should have them done soon.

…d tests

Note: this only moves middleware functions that have associated tests.
@hswolff
Copy link
Contributor Author

hswolff commented Nov 8, 2013

So I think I got as far as I could with this. I didn't move any middleware functions that I wasn't sure how to create tests for.

@ErisDS let me know if there's any revisions this may need. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants