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

Added CoffeeScript support #150

Merged
merged 8 commits into from May 31, 2012
Merged

Added CoffeeScript support #150

merged 8 commits into from May 31, 2012

Conversation

larzconwell
Copy link
Contributor

This simply checks for .coffee files then it tries to require the module with a error message if it isn't found

This is discussed in issue #77

@larzconwell
Copy link
Contributor Author

@mde I'll look into the controller and model loading, and Node caches modules so it doesn't actually re-require it, I'm not sure of another way to do it.

@techwraith Yes, just requiring it, will make it so coffee files and js can be used.

@mde
Copy link
Contributor

mde commented May 30, 2012

Yes, I'm aware of the module caching. If the require returns a value, just set it once. An existence check is more explicit and probably faster than making a function call. Something like:

var coffee;

coffee = coffee || require('coffee-script');

@techwraith
Copy link
Contributor

@larzconwell It does cache modules, so it shouldn't be an issue, but the better way to do it would have been to call a function where you require coffee currently that keeps track of whether it's been required already and only require it if it hasn't. Not a big deal though.

@techwraith
Copy link
Contributor

@mde that's even better.

@larzconwell
Copy link
Contributor Author

Ah okay that's a good idea guys, I'll use the || check!

@larzconwell
Copy link
Contributor Author

when loading the models and controllers it calls the _getDirList function so it'll always make sure Coffee is required so I think this should be enough. I'm not sure where else we should check for Coffee, but I'm still looking around

@larzconwell
Copy link
Contributor Author

Okay yeah this should work with coffee models and controllers, because for the _registerModels and _registerConstructors functions, they both call _getDirList which returns an array of objects with the path, etc. but also requires Coffee if ,coffee extensions are found, then back in the main function it simply requires the filePath in the array.

I'm gonna make a test app to make sure (:

@larzconwell
Copy link
Contributor Author

Should we add a check for the router as well?

@techwraith
Copy link
Contributor

@larzconwell Yeah, that would be good. If someone wants their entire app to be in coffeescript, they should be able to do it that way. Might want to do that for model adapters as well (if that's not already covered).

@larzconwell
Copy link
Contributor Author

I'm just gonna go through it all and see where a check is needed!

@techwraith
Copy link
Contributor

@larzconwell Let me know when it looks good to you, I'll check over it with @mde and merge it in.

@larzconwell
Copy link
Contributor Author

So I've went through app.js and made it so controllers, models, the init file and the router files can all be written in CoffeeScript.

There's some weird bug though, when controllers(Haven't tested models yet) are in CS, it can't get the actions correctly. it comes down to line 462 in app.js where we are creating a new instance of the specific controller, when in CS it doesn't gather the actions. Maybe I am writing my CS wrong but it looks exactly the same as the normal JS version.

Main = ->
  this.index = (req, res, params) ->
    this.respond params,
      format: 'html'
      template: 'app/views/main/index'

exports.Main = Main

@techwraith
Copy link
Contributor

Try this:

Main = ->
  index = (req, res, params) ->
    this.respond params,
      format: 'html'
      template: 'app/views/main/index'

exports.Main = Main

@techwraith
Copy link
Contributor

Actually, put an @ in front of the index function. @index

@techwraith
Copy link
Contributor

@larzconwell Look at Function binding in the CoffeeScript docs.

@larzconwell
Copy link
Contributor Author

I thought that might of been the problem as well, but nope still nothing.

I figured it out! For CoffeeScript you have to do

class Main
  index: (req, res, params) ->
    this.respond params,
      format: 'html'
      template: 'app/views/main/index'

exports.Main = Main

I have a feeling their will be a lot of issues about this because it's not that obvious it needs a class.

@techwraith
Copy link
Contributor

@larzconwell Can you add an example coffeescript app in the examples directory so that we can at least have somewhere to point people to?

@larzconwell
Copy link
Contributor Author

Ah yes I will! I'll be done soon, I just need to make it so the environment scripts can be CS, then test the models and controllers more. Also write an example app.

@techwraith
Copy link
Contributor

@larzconwell If you can, just translate the Todo app into CS.

@larzconwell
Copy link
Contributor Author

There we go! Everything can be written in CS now including main configuration, models, controllers, etc.

Model adapters can be written in CS or JS, since they are required in the apps init file the user will have to require CS themselves, but if the init is written in CS, then it will already be included.

@larzconwell
Copy link
Contributor Author

Sorry about the mega log additions

@techwraith
Copy link
Contributor

That's the old way of doing model adapters. The new way automatically requires them from 'models/adapters/'.

Sent from my iPhone

On May 30, 2012, at 10:03 PM, Larz Conwellreply@reply.github.com wrote:

There we go! Everything can be written in CS now including main configuration, models, controllers, etc.

Model adapters can be written in CS or JS, since they are required in the apps init file the user will have to require CS themselves, but if the init is written in CS, then it will already be included.


Reply to this email directly or view it on GitHub:
#150 (comment)

@OscarGodson
Copy link
Member

damn @larzconwell quite the pull request! I'm not much a CoffeeScript guy, but rewriting the examples and adding support is quite impressive! 👏

@larzconwell
Copy link
Contributor Author

I'm re-writing the example now @techwraith, So currently the only built in adapter is mongo(/lib/model/adapters/mongo.js) right? Others people create will go into /app/models/adapters? I'm just making sure I got the new way down correctly (:

@OscarGodson haha Thanks!

@techwraith
Copy link
Contributor

Yep, that's right! Sorry for the lack of documentation on that, they just landed a few weeks ago.

Sent from my iPhone

On May 31, 2012, at 9:10 AM, Larz Conwellreply@reply.github.com wrote:

I'm re-writing the example now @techwraith, So currently the only built in adapter is mongo(/lib/model/adapters/mongo.js) right? Others people create will go into /app/models/adapters? I'm just making sure I got the new way down correctly (:

@OscarGodson haha Thanks!


Reply to this email directly or view it on GitHub:
#150 (comment)

@larzconwell
Copy link
Contributor Author

There we go! I based it off the mongo example this time, so it uses the mongodb for saving.

@larzconwell
Copy link
Contributor Author

Oh man, I forgot to add the .coffee support for model adapters, i'm getting to that now

@larzconwell
Copy link
Contributor Author

Btw when this is completely done, I'm gonna squash all the commits, so they don't mess up the log (:

@larzconwell
Copy link
Contributor Author

Okay I think this is done! Let me know if anything should be changed

@techwraith
Copy link
Contributor

Looks good to me @larzconwell!

@mde I think we can merge this in, what do you think?

@mde
Copy link
Contributor

mde commented May 31, 2012

If all tests still pass then yes, let's do it. It looks amazing. :) @techwraith I'll let you do the honors. Awesome work, @larzconwell!

@larzconwell
Copy link
Contributor Author

They pass! :D I'll squash the commit now, thank you guys!

@larzconwell
Copy link
Contributor Author

I'm not sure how to squash the commits /:

@techwraith
Copy link
Contributor

@larzconwell It's fine, I don't think a squash is needed. There are only 8 commits here :)

techwraith added a commit that referenced this pull request May 31, 2012
@techwraith techwraith merged commit 852931d into geddy:master May 31, 2012
@techwraith
Copy link
Contributor

Merged in, thanks @larzconwell!

@larzconwell
Copy link
Contributor Author

Awesome awesome! In a little while i'll open a PR for the template system.

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.

None yet

4 participants