search

alarner / perk

A well documented set of tools for building node web applications.
http://perkframework.com
MIT License
181 stars 31 forks source link

Log dev server info on npm run dev & specify dev environmen #38

Closed tphdev closed 8 years ago

tphdev commented 8 years ago

@alarner I think this is what you were after. Let me know if I'm making a PR to the right branch.

alarner commented 8 years ago

@t3patterson you are crushing it this weekend!

Here are a few minor comments. Your code works and is well written so don't be put off by the wall of text here. I just wanted to explain my reasoning behind each of these requests.

  1. Normally you'll want to submit the PR to the master branch. The last PR you did was a little bit unique because I created the test case first on a new branch, so it made sense to merge into there. Could you re-submit this PR to the master branch? I think if you are working from the master branch you shouldn't need to modify routes/index.js because that change is already on master but these two commits are what we want:
  2. https://github.com/alarner/perk/pull/38/commits/596dd4bce0445388e9fafe0beff88a289b71eaa0
  3. https://github.com/alarner/perk/pull/38/commits/739551a9d75b056a38352b7832917ea2e05a4a53
  4. I was thinking more about adding NODE_ENV=production to the start script and I think that could cause problems with Heroku deployment. When we deploy to heroku it's expecting NODE_ENV=heroku but uses that same npm start command to boot up the server, so forcing NODE_ENV=production will likely break Heroku deploys. Sorry that's my bad. I should have thought about that earlier. The change to npm run dev looks perfect though.
  5. There are a couple stylistic mistakes that you wouldn't have known were problems. If you pull from master before issuing the new PR you can run the command npm run eslint to see any problems with your code.
  6. I resolving two issues in the same PR is great. If you reference them in the description like: "Resolves #30 and #34" it will make it clear which ones so that I can mark them resolved when we merge in the PR.
  7. It would be slick if you could figure out a way to display the Server is running at ... message after the first sass compilation, js compilation and server are booted up. I'd recommend using async (which is already a dependency) inside of the bin/build file. Each of the functions loader(...), sass(...) and server(...) have a callback argument so if you wrap them inside of async.parallel you can be sure they are all complete and then display your message.

This is awesome! Thank you so much for the help! If you have any questions about my comments let me know. I'm happy to clarify anything that's cryptic.