Make the new Auth stuff work with Jade and Handlebars #251

Closed
Techwraith opened this Issue Nov 3, 2012 · 16 comments

Projects

None yet

3 participants

@Techwraith
Contributor

No description provided.

@djensen47

I've started to translate the view in geddy-passport to jade but where should they live? In the same dir as the .ejs files or a subdir?

Let me know and I'll submit a pull request with the translated files.

@larzconwell
Contributor

Currently we install geddy-passport from npm then copy all it's files into the app. So we would manually have to check if it's a view file and then decide what engine to use during the copy loop.

@djensen47 you should probably do this to make it easy for us to check the engine:

app/
  views/
    ejs/
      <views in EJS>
    jade/
      <views in Jade>
    handlebars/
      <views in Handlebars>
    mustache/
      <views in Mustache(should be the exact same as handlebars)>

EDIT: I'm doing the changes to the Jakefile to correctly copy the views, so if you want you can just create the view templates.

@larzconwell
Contributor

Okay an update here, in geddy-passport I put the ejs views in their own directory in views. Here's the commit: geddy/geddy-passport@4aa861c So you can use that as a base to do jade, handlebars/mustache.

Also I implemented the engine specific view copies in Geddy the commit is here c189922

@djensen47

Excellent, I should be able to work on this tonight.

…time to learn handlebars.

@larzconwell
Contributor

I can help if you want you need any, just let me know and I'll jump on irc!

@Techwraith
Contributor

Is there a way that we can not duplicate handlebars/mustache code? They have the same syntax, so lets try not to repeat ourselves.

Sent from my iPhone

On Jan 15, 2013, at 10:02 AM, David Jensen notifications@github.com wrote:

Excellent, I should be able to work on this tonight.

…time to learn handlebars.


Reply to this email directly or view it on GitHub.

@larzconwell
Contributor

Yeah definitely, should I completely remove mustache specifics from the code and make it act like handlebars under the cover? The only thing really need to keep is the cli option.

@Techwraith
Contributor

Sounds like a plan to me :)

Sent from my iPhone

On Jan 15, 2013, at 10:53 AM, Larz Conwell notifications@github.com wrote:

Yeah definitely, should I completely remove mustache specifics from the code and make it act like handlebars under the cover? The only thing really need to keep is the cli option.


Reply to this email directly or view it on GitHub.

@larzconwell
Contributor

Okay so I've completely removed the code for mustache explicitly it's now just handlebars. Should I make a console.log when they have the --mustache option? Because the generators all use .hbs now and I'm sure they'd be confused. Or maybe include a note on the help dialog?

@Techwraith
Contributor

This is silly, but perhaps we should just switch the file extensions. I know that means a slight rewrite of the template generation code, but it may be worth it.

Sent from my iPhone

On Jan 15, 2013, at 12:43 PM, Larz Conwell notifications@github.com wrote:

Okay so I've completely removed the code for mustache explicitly it's now just handlebars. Should I make a console.log when they have the --mustache option? Because the generators all use .hbs now and I'm sure they'd be confused. Or maybe include a note on the help dialog?


Reply to this email directly or view it on GitHub.

@larzconwell
Contributor

Can do

@Techwraith
Contributor

I realize now that I may have been misleading - I meant, depending on what the user inputs, use the right file extension.

@larzconwell
Contributor

Yeah I got you, at first I thought you ment change all the hbs extensions to ms then thought that was a bit silly so I assumed you ment change extension when using mustache.

@larzconwell
Contributor

This is looking pretty ugly and unmaintainable. Here's an example from the code that generates the app.

Before

cps.forEach(function (cp) {
  jake.cpR(path.join(basePath, cp[0]), path.join(name, cp[1]), {silent: true});
});

After

cps.forEach(function (cp) {
  if (cp[1] === 'app/views' && engine === 'mustache') {
    cp[0] = cp[0].replace(engine, 'handlebars');
  }
  jake.cpR(path.join(basePath, cp[0]), path.join(name, cp[1]), {silent: true});

  if (cp[1] === 'app/views' && engine === 'mustache') {
    engine = 'handlebars';
    // loop through dirs in path.join(name, cp[1])
    // change all file name extensions
    // recurse through child dirs
  }
});

What do you think? If you think it's not so bad, I'll keep going but it's pretty bad for maintenance, and future changes.

@Techwraith
Contributor

Preferably we'd maintain a dictionary somewhere that maps template engines
to file extensions and we'd use that to assign a file extension for all
templates. You're right though, this is starting to look like spaghetti
code, I don't think it has to be so messy though.

On Tue, Jan 15, 2013 at 4:36 PM, Larz Conwell notifications@github.comwrote:

This is looking pretty ugly and unmaintainable. Here's an example from the
code that generates the app.

Before

cps.forEach(function (cp) {
jake.cpR(path.join(basePath, cp[0]), path.join(name, cp[1]), {silent: true});
});

After

cps.forEach(function (cp) {
if (cp[1] === 'app/views' && engine === 'mustache') {
cp[0] = cp[0].replace(engine, 'handlebars');
}
jake.cpR(path.join(basePath, cp[0]), path.join(name, cp[1]), {silent: true});

if (cp[1] === 'app/views' && engine === 'mustache') {
engine = 'handlebars';
// loop through dirs in path.join(name, cp[1])
// change all file name extensions
// recurse through child dirs
}
});

What do you think? If you think it's not so bad, I'll keep going but it's
pretty bad for maintenance, and future changes.


Reply to this email directly or view it on GitHubhttps://github.com/mde/geddy/issues/251#issuecomment-12298010.

@larzconwell
Contributor

Yeah that sounds like a good idea to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment