Skip to content
This repository has been archived by the owner on Sep 18, 2024. It is now read-only.

Feature 211 articles #212

Closed
wants to merge 6 commits into from
Closed

Conversation

hefox
Copy link

@hefox hefox commented Feb 15, 2017

Adding articles controller example to mvc, #211

It's not ready for merge, figured better to commit early and see feedback to make sure going right direction. Never done a generator before.

Some pending issues:

  • No coffee script yet (don't have much experience in that and figure get the non-coffee right first).
  • Using _id to link to articles/[id] in the views, but that's mongodb specific, need to figure out what the IDs for each are.
  • I have only used mongodb with node/express so am unsure my syntax for the other databases are correct, specifically for the findOne methods (though I googled each).
  • I suspect the articles views may be getting copied to basic, ideally they would not be. It looks like copytpl doesn't have an exclude, so possible either need to hardcode what files are copied for basic or figure out how to loop through only top level files.

I've done a simple test where where generated a project and verified articles returned the No Articles yet message.

if (err) return next(err);<% } %>
<% if(options.database == 'mysql' || options.database == 'postgresql' || options.database == 'sqlite'){ %>
db.Article.findById(req.params.articleId).then(function (article) {<% } %><% if(options.database == 'rethinkdb'){ %>
Article.filter(r.row('id').eq(req.params.articleId)).run().then(function (article) {<% } %><% if(options.database == 'none'){ %>
Copy link
Owner

Choose a reason for hiding this comment

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

With Thinky you can use Article.get(req.params.articleId) see: https://thinky.io/documentation/api/query/

<% if(options.viewEngine == 'marko'){ %>
var indexTemplate = marko.load(require.resolve('../views/articles/view.marko'));<% } %>
router.get('/:articleId', function (req, res, next) {<% if(options.database == 'mongodb'){ %>
Article.findOne({ '_id': req.params.articleId }, function (err, article) {
Copy link
Owner

Choose a reason for hiding this comment

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

@petecoop
Copy link
Owner

Hey looking good so far!

  • Yeah I agree with doing normal js first then coffeescript, I'm not personally a fan of coffeescript and actually not sure how many people are using it now vs latest ES6/7.
  • I've put some comments on the queries, linking to the relevant docs. You shouldn't need to worry about the id's, I think they assume you use id or the equivalent for the DB, and if it were to change it'd be in the model - if you use the right methods it handles that for you.
  • I think you'll need to create an mvc-views folder or something, which gets copied over after the normal views.

@hefox
Copy link
Author

hefox commented Feb 17, 2017

Thanks! I believe I've updated everything, the queries and the mvc-views.

I suspect so about coffee script, perhaps drop support for it? Since it's a generator, projects using it have already been generated so don't need to worry about upgrade path?

@petecoop
Copy link
Owner

I suspect so about coffee script, perhaps drop support for it? Since it's a generator, projects using it have already been generated so don't need to worry about upgrade path?

While I'd like to, someone put some effort in and contributed coffeescript. If someone were to upgrade the generator they might then be surprised to find it won't generate coffeescript anymore.

Unfortunately I don't have any data on how many people use it :/

I will check on your latest when I've got a chance

@petecoop
Copy link
Owner

petecoop commented Mar 7, 2017

Hey sorry I've taken a while on this, the code looks good but I just need to test actually running it which is something the tests don't really do. Have you tested running the coffeescript stuff locally?

@petecoop petecoop closed this Oct 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants