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

Seperate out app and serving logic #51

Closed
wants to merge 2 commits into from

Conversation

plwalters
Copy link
Contributor

Fixes #43

bin/www Outdated
@@ -0,0 +1,114 @@
#!/usr/bin/env node
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I basically took the default bin/www that express generates and moved the listening on ports stuff over as-is in to the file

Copy link
Owner

Choose a reason for hiding this comment

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

Can I still use nodemon on the server?

Copy link
Contributor Author

@plwalters plwalters Oct 3, 2018

Choose a reason for hiding this comment

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

Yes, I'll verify again but this is how I typically set up my start script -

  "start": "npm install && nodemon ./bin/www",

To show that it should be the exact same way to run it with nodemon

Copy link
Owner

Choose a reason for hiding this comment

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

you need to chmod the www file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't that I know of

server.js Outdated
const unlockDelayMs = 1000;
const unlockerLnd = localLnd({is_unlocker: true});

if (NODE_ENV !== 'production') {
walnut.check(require('./package'));
}

module.exports = app;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

export the app. I'm testing now the effect of doing it this way on setting up the routes in the http tests, if you have any feedback on doing this different just lmk

Copy link
Owner

Choose a reason for hiding this comment

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

does this mean it would be possible to have both ways of running the server? the bin/www way and the original way as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic that was there before is just moved in to the www file to be a node script that is run when actually serving the code.

By separating it out the app can be set up independently without actually turning on the web server and listening to the port, this allows libs like supertest to run it and test out the end points in an integration test without having it running and calling the api manually.

HTTP tests coming in a separate PR, wanted to put this up for discussion first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe it would be nicer if running nodemon server.js still does the same thing it did before but its internals change to use an abstracted server that also is used in the www/bin and for testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about -

$ mv ./server.js ./app.js
$ mv ./bin/www /.server.js

Then the command wouldn't even change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR to show what I mean

Copy link
Owner

Choose a reason for hiding this comment

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

I don't 100% understand the change but if you add a test I can mess around with things and try and make sure the test still passes

@plwalters plwalters closed this Oct 4, 2018
@plwalters plwalters deleted the app branch October 4, 2018 18:16
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