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

Node organization #4

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Node organization #4

wants to merge 7 commits into from

Conversation

devspacenine
Copy link
Collaborator

Organized the routes so that its easier to add new ones and added a directory for middleware. Also, did a few cleanup things.

app.set('rootPath', __dirname);
// Global Paths
app.set('paths', {
routes: path.join(app.get('rootPath'), 'routes'),
Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain a little bit of what is going on here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The use of just one const keyword is an optimization trick for v8.

The app.set() functions are creating global values that can be accessed anywhere you have a reference to the app obect.

Copy link
Owner

Choose a reason for hiding this comment

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

Is this like a best practice or something? It seems weird to me to have to pass around the app everywhere. I would think it would be simpler to have each module completely encapsulate all of its requirements.

Copy link
Collaborator Author

@devspacenine devspacenine Aug 24, 2016

Choose a reason for hiding this comment

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

No, I just pass it to routes and middlewares so that each module has the option to attach to either the global app or the apiRouter. That way can can do things like write middleware that only applies to certain routers (e.g. auth middleware for api endpoints). I'm not sure how you could write a middleware that applies to all endpoints without passing the app object. Not to say there isn't a way, though.

Copy link
Owner

Choose a reason for hiding this comment

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

I think the way I'm doing that in the existing api_routes would effectively accomplish that same feat. If I wanted something to apply to all api routes, I could just specify app.use('/api', middleware) . Then all calls to the api routes would pass through that middleware right?

require(route)(app, apiRouter);
});

app.use('/api', apiRouter);

app.get('/users', function(request, response) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't put these guys in their own module yet, because I wasn't sure what your plan is with them.

Copy link
Owner

Choose a reason for hiding this comment

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

Those were just some garbage I threw together as an example. I'd actually go ahead and remove them at some point.

@devspacenine
Copy link
Collaborator Author

Went ahead and removed the user endpoints. And I changed the static folder to something other than root. That way all of the node code and config files aren't served.

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.

2 participants