alarner / perk

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

root-index-route breaks when switching to ejs templating engine #28

Closed tphdev closed 8 years ago

tphdev commented 8 years ago

in routes/index.js res.render needs 2nd argument (object with title property)

router.get('/', function(req, res, next) {
    res.render('index'  /*, { title: ... } */); 
});
alarner commented 8 years ago

Hey @t3patterson. Thanks for the bug report. I removed the title parameter from the default index view and replaced it with a hard coded title in this commit: https://github.com/alarner/perk/commit/2ac840018d3d8ef3d880911f35381b49dba47857

Did you run into a problem where the view was expecting a title but it didn't exist? Was it from using the perk-cli after that commit was made (18 days ago)? If so, this could actually be a bug in the cli improperly caching old files or something and we can take a look into that.

If this problem originated from a perk install that you started more than 18 days ago then it's likely that you're just using some outdated code, but it shouldn't be an issue that others run into.

tphdev commented 8 years ago

@alarner Hey sure, no problem.

Yes, the error happened when I generated a project today (6/15/2016) from perk-cli and then changed the template engine to ejs in app.js. For me, the perk-cli is still generating index.ejs, error.ejs, and dashboard.ejs files in the /views directory

alarner commented 8 years ago

@t3patterson this was a really good catch 💯. Thank you for letting me know about the issue. It turns out that this problem is not where it seems to be (as many bugs are). The issue has to do with the perk-cli. I've written up an issue that explains the problem and also added a test case (which is currently failing) demonstrating the problem. If you're interested in tackling it I've tried to provide good instructions for how to make a fix. Feel free to make PR to the perk-cli artifact-file-fix branch if you're interested in tackling this. If not no worries, I can fix it up too, but I wanted to give you the chance since you found the bug :)

Description of the bug: https://github.com/alarner/perk-cli/issues/2

tphdev commented 8 years ago

@alarner Cool, glad it was useful. And yes, I'd like to take a crack at it!

My git skills are not very strong since this is the first open source project I've made contributions too. I'm supposing you want me to pull the branch artifact-file-fix into a local repo on my forked perk, make the changes ( and have the test cases pass), push them to my fork, and make a PR to your artifact-file-fix branch?

alarner commented 8 years ago

Yes that would be perfect. Let me know if you run into any snags or want any guidance @t3patterson

alarner commented 8 years ago

@t3patterson any questions so far? Happy to set up a time to skype if you want to walk through anything.

tphdev commented 8 years ago

@alarner took a stab 2 weeks ago and was very demoralized because i couldn't get any traction.

took a fresh look this morning, and got the test to pass by creating a clearLocation(locations.extractPath) method that executes right after ensureDir(locations.tmpPath) in src/steps.js

not sure if this is an acceptable solution because node and server side development is still quite foreign to me

alarner commented 8 years ago

@t3patterson This looks like exactly what I was thinking: https://github.com/t3patterson/perk-cli/commit/2794c73664c941a7cf6651d1961bdde9dc2d0142 You did it perfectly!

alarner commented 8 years ago

Fixed in https://github.com/alarner/perk-cli/pull/5. Thanks @t3patterson!