pthrasher / snockets

Sprockets-style script concatenation for Node
119 stars 39 forks source link

coffeescript closure pains #34

Open jescalan opened 11 years ago

jescalan commented 11 years ago

I've found that I have an issue with the way that coffeescript plays with asset pipeline libraries.

As an example, when dealing with a larger project, sometimes you'll want to split your js into multiple files then concat them at the end (exactly what snockets/sprockets is for), but when using coffeescript, there's hardly any use to concatenating your files since the extra closure on compile ensures that no code can be shared between the two files anyway. The only way around this issue is to attach shared functions to the window object, which is exactly what the extra closure is trying to prevent.

Ideally, I would want to have a single closure wrapper that I can put in myself inside an app.coffee file, for example, then inside that closure I could require all my other files which are split out for organization purposes, and have all the files compile bare (without the closure). This way, it's easy to organize code cleanly, export functions/variables from the different files, and all your code is still protected by an anonymous function closure. Ah, the dream.

So I set about seeing if I could make this happen. Initially I thought it would be easy to fork snockets and add an extra option on initialization (like bare: true) which would add the {bare: true} option into the coffeescript compile function. I did this, and it was just fantastic and only like 2 lines of code. But when I tested it, I found a rather strange result, illustrated below:

# main.coffee

->
  #= require 'other'
  console.log 'hello from the main file!'

# other.coffee

console.log 'hello from the other file!'

This is the coffeescript code I started out with, figuring that if things went magically well, I would end up with this result as the compiled js:

# main.js (imaginary)

(function(){
  console.log('hello from the other file!')
  console.log('hello from the main file!');
});

...however, what I actually ended up with was this:

# main.js

console.log('hello from the other file!')

(function(){
  console.log('hello from the main file!');
});

Now, I can totally understand why this is happening, and I feel like fixing the way the concatentation through requires works might be a pretty large a fundamental change to the library, with other possible consequences I didn't think of, as I haven't had nearly as much experience building asset pipeline libraries as you have. But I just wanted to run it by you to see your thoughts. Would this be something worth me looking into submitting a pull request for? Would it be something maybe better suited to be built as it's own tiny coffeescript requires library? Did this explanation not even exactly make sense to you and I should try re-writing this since I suck at writing? Either way, let me know!

Cheers, and thanks for reading through this epically long and ridiculous issue! : )

EDIT: Just saw this #23, read it through, and that probably is a better solution than my forked version, but either way this doesn't really change my overall issue

pthrasher commented 11 years ago

@jenius I'm going to close this issue since the issue you referenced is open. Let's move discussion there.

jescalan commented 11 years ago

What actually happened to the issue I linked?

pthrasher commented 11 years ago

Your link no longer works because it was a full link to trevor's repo. This is the new master repo. I've edited your issue description to just have the #23 in it. That said, this pull request was previously closed by Trevor, and for some reason I cannot reopen it. I'm going to re-open this one as a reference.

jescalan commented 11 years ago

Ok sweet. So I know what I wrote is super long and you probably didn't read through the whole thing as well (I probably wouldn't either if I had just picked up maintenance and had a ton of issues to go through), but my reference to #23 was not that it was an exact duplicate, just that it had some similar features.

What I think I was getting after at the time was essentially that the requires are sort of hoisted up to the top of any file, rather than dumped in the place that the require happened. There are a few examples in the original issue of how this happens. This was more of a probing if it were safe/possible without a rewrite to have require statements dump the script exactly where the require is placed than a specific problem with the library. I do think this would be an exceedingly useful feature, as it would make it possible essentially to have partials in your javascript, much like we frequently do in html

pthrasher commented 11 years ago

@jenius I definitely think this could be a useful feature.

The reason it doesn't work this way now, is due to building a dependency tree. If two files require the same alternate file (if a.coffee and b.coffee both require c.coffee) c.coffee would be above the other two 100% of the time instead of being inlined twice. This is because snockets is meant first and foremost as a dependency management tool.

I could see a use case for what you describe though. Perhaps a new directive like "insert", "import" or "include" would be good to have.

jescalan commented 11 years ago

Ah got it - I figured there has to be some design reason behind that decision. I do think it would be cool to have something like insert - worth trying to open up a pull request for it?

pthrasher commented 11 years ago

@jenius #14 seems to be on the same page with you. Definitely check that issue out. I'm currently working on the next version (2.0.0) and while this new directive won't make it into 2.0.0, perhaps 2.1.0

I'm trying to rapidly add new functionality, and the 2.0.0 update is going to make this codebase a bit more spread out, and easier to maintain (therefore, easier to contribute)

I'm planning to be done with 2.0.0 by the end of this month (may)

jescalan commented 11 years ago

Awesome, sounds like you're really taking this project in the right direction, which is fantastic. I'll be on the lookout for 2.0.0 to read through the new codebase