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

Adding Snockets compilation of js, cs and ts files #375

Closed
wants to merge 1 commit into from

Conversation

D1plo1d
Copy link
Contributor

@D1plo1d D1plo1d commented May 8, 2013

This patch adds snockets support to sails via asset-rack. The major issue this addresses is dependency management in client side code.

From my perspective #= require "my_dependency" has always been very readable, predictable and concise.

But I wonder if there are different camps on this. What's the consensus?

@colinwren
Copy link
Member

The plan right now is to move from asset-rack to grunt for managing front end assets. See #240 for more on this.

Feel free to join the #sailsjs IRC channel on freenode if you have any questions.

Thanks for your recent contributions!

@colinwren
Copy link
Member

Whoops! Didn't see you already mentioned that issue.

You wouldn't have to really learn yeoman/grunt for basic usage, out of the box it would support most of what asset-rack supports right now. If you do spend a little time with it, it is super configurable and you wouldn't need to modify asset rack or sails to get things like snockets or stylus because there are grunt tasks for them.

@D1plo1d
Copy link
Contributor Author

D1plo1d commented May 11, 2013

Thanks! I'll have to look into Yeoman and Grunt a little more. I think it's safe to say I don't have enough experience with either of them at this point.

What I'd really like to do though is get Sails working better out of the box. Are these changes ok as an interim solution?

@greeze
Copy link
Contributor

greeze commented May 11, 2013

I'm here in search of a way to minify and concatenate assets in a specific order, but I'm new to Snockets. Is this a solution to that problem? Do you have an example of how this might work? I'm ready to pull this in to my fork even if this doesn't make it into the official repo. I'm kind of desperate for a decent solution.

@D1plo1d
Copy link
Contributor Author

D1plo1d commented May 11, 2013

@greeze So concatenation works out of the box. Just add //= require "whatever_directory/my_dependency" and it will concatinate my_dependency.js (or .coffee, etc.) and the current file.

I haven't tried minifying Sails assets but the snocket assets should work the same as any other asset once it's concatenated by snockets so I think (and I could be wrong here) that Sails will automatically minify your assets when sails.config.environment is set to production.

@greeze
Copy link
Contributor

greeze commented May 11, 2013

Excellent. I'm going to fool around with this right now.

Unfortunately, I don't think Sails automatically minifies assets that are pulled in from views, even in production. I tested by adding a script src to one of my jade templates and pulling in a .js file from the /public directory. Even when I started the server with --prod the file wasn't minified. Yours looks like a totally workable solution until a final decision is made. Thanks for doing this!

@D1plo1d
Copy link
Contributor Author

D1plo1d commented May 11, 2013

@greeze No problem. Let me know if you figure out how to minify your assets, I'm on course to run into the same problem in about 7 days!

@greeze
Copy link
Contributor

greeze commented May 11, 2013

In other news, Github should add a feature to easily merge pull requests into different forks.

Edit: Also, this appears to work flawlessly. Thank you!

@dcbartlett
Copy link
Contributor

@D1plo1d, can you resubmit your pull requests to the development branch please.

@D1plo1d
Copy link
Contributor Author

D1plo1d commented May 13, 2013

@dcbartlett Fixed. Sorry about that, I'll update my other pull requests as well.

moved to: #384

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

Successfully merging this pull request may close these issues.

4 participants