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

Flag to disable helpers during compilation (#1494) #1820

Closed
wants to merge 5 commits into from
Closed

Flag to disable helpers during compilation (#1494) #1820

wants to merge 5 commits into from

Conversation

TrevorBurnham
Copy link
Collaborator

As discussed at #1494, this pull request allows you to tell CoffeeScript.compile that you don't want helpers/utilities like __hasProp to show up in the compiled output. That way, if you have a large number of modules that you're compiling for front-end deployment, you can just put the helpers in one file and eliminate them in the other. This was discussed briefly on Twitter today:

http://twitter.com/#!/ryanflorence/status/132122428913614849
http://twitter.com/#!/CoffeeScript/status/132156500364894208
http://twitter.com/#!/jashkenas/status/132161592451268608

Example:

coffee> CoffeeScript.compile 'x in arr', bare: true
'var __indexOf = Array.prototype.indexOf || function(item) {\n  for (var i = 0, l = this.length; i < l; i++) {\n    if (this[i] === item) return i;\n  }\n  return -1;\n};\n__indexOf.call(arr, x) >= 0;'
coffee> CoffeeScript.compile 'x in arr', bare: true, utilities: false
'__indexOf.call(arr, x) >= 0;'

This does not affect the coffee command. It's purely to give other compilation tools more flexibility.

@michaelficarra
Copy link
Collaborator

I fail to see the necessity of this flag. Only the necessary helpers are included. If you're compiling multiple files, combine them first, right?

@jashkenas
Copy link
Owner

As helpers are not visible between different wrapped closures, this will simply cause all of the code within your disabled-helpers file to break ... unless compiling all files --bare, correct?

@TrevorBurnham
Copy link
Collaborator Author

If you're compiling multiple files, combine them first, right?

That's what I thought at first, but let's say that you're using something like Sprockets. You want to compile and concatenate a.coffee, b.js, and c.coffee in that order, with only one copy of the utility functions that a.coffee and c.coffee both require. That's a very hard thing to do at present. This flag would make it much, much easier.

Indeed, the Rails folks will probably want to make "insert global CoffeeScript utility functions if any .coffee files are in the pipeline" a default, since having several declarations of __extends and such in the application JS has become the norm.

@michaelficarra
Copy link
Collaborator

@TrevorBurnham: How do they know

  1. Which helpers are needed by the code
  2. The current source of the particular helper

This feels like a half-hearted approach. Either we make that information available for compilation tools like sprockets or this flag is unnecessary.

edit: wording

@TrevorBurnham
Copy link
Collaborator Author

As helpers are not visible between different wrapped closures, this will simply cause all of the code within your disabled-helpers file to break ... unless compiling all files --bare, correct?

Or declare the helpers in global scope yourself. I'd suggest making a utilities.js file part of the repo, under extras. In fact, adding that to the pull request now...

Which helpers are needed by the code

Just throw them all in. You might wind up with one or two things you don't need, but that's an O(1) efficiency loss rather than O(n) with respect to the number of .coffee files. This is exactly what was requested at #1494. As @lucaswoj said when opening the issue:

I'd love to just have a single file of all CoffeeScript helpers and include that instead.

And as @rpflorence said just today,

I'd be ecstatic with nothing more than an exclude helpers flag. I can put them on the page myself.

@TrevorBurnham
Copy link
Collaborator Author

@michaelficarra Right, that was for debugging. Removed.

"__#{utility} = #{func()}"
).join(',\n') + ';'
fs.writeFileSync 'extras/utilities.js', code
consoole.log "built utilities.js"
Copy link
Collaborator

Choose a reason for hiding this comment

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

haha consoole? I'm guessing you didn't test this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:'(

@ryanflorence
Copy link

I'm surprised more people don't find this needless code disturbing :\ As I mentioned before in #1494, it's impractical to expect your entire app to be transpiled at once. All of our JS uglified and gzipped is over 1 megabyte (!), it'd be a crime to expect our users to download and execute that much JS.

Consequently, we have a script that compiles a single file from cs to js, and also watches the file system as we develop to do it seamlessly. We then use the RequireJS optimizer to build layers of our application that are loaded when they are needed. In a large AMD application, if you do it right, you have a lot of individual coffeescript files.

My hand-written JS is smaller than what coffeescript kicks out. Add in the helpers (which show up in nearly every module) and now that 1 megabyte is creeping up on 1.5. We don't need hundreds of __bind, __extend, __hasProp, etc.

Anyway, anybody using this option should have a clue, and the tool can provide feedback saying "make sure you include the helpers some how!" I know it sounds gross, but is there really an alternative w/o making the coffeescript command-line tool a robust application packager / optimizer?

I'm happy to toss the 4-5 helpers in the global scope and be done with it. If you're using this option, you don't have to worry about which ones you need, just dump them all in.

@erisdev
Copy link

erisdev commented Nov 5, 2011

👍 I may or may not have been begging for something like this, but I sure as hell want it. I think it would be cleaner to namespace the helpers, though, if they are going to be in the local scope. CoffeeScript.__indexOf, anyone?

@ryanflorence
Copy link

The output would have to check if the namespace is defined yet before adding the necessary helpers. This adds more code to the output :\

var __coffee = __coffee || {};
__coffee.extend = function (){
  /* extend definition */
}

Sounds nice, but I don't think it's very important. If you're turning off the helpers, you just need to know that you've got to put them in your app, and if you're concerned about globals, you just need to scope them manually.

@erisdev
Copy link

erisdev commented Nov 5, 2011

@rpflorence I was thinking something more along the lines of this in the utilities.js

var __coffee = {};
__coffee.extend = function() {
  …
}

and then scripts compiled "bare" would use it as __coffee.extend. No need to check if the namespace exists since we're already trusting the end user to be clueful, right?

@TrevorBurnham
Copy link
Collaborator Author

I think it would be cleaner to namespace the helpers, though

I suspect that the extra cruft from the namespacing would kill the efficiency gains from eliminating duplicate declarations in most cases. At any rate, I don't know of anything that likes to use the __ prefix for its globals.

@erisdev
Copy link

erisdev commented Nov 5, 2011

@TrevorBurnham You make a good argument, although I might humbly suggest a prefix of cs$ or similar.

IIRC, the ECMAScript standard suggests that $ should be reserved for compiler-generated identifiers, so this is a reasonable application. I know jQuery breaks the rules by using $ on its own, but I haven't seen anybody using cs$ and it would be a little more obvious that it's "ours". C:

@tazsingh
Copy link

tazsingh commented Nov 5, 2011

Awesome. This would definitely help with stuff like this https://gist.github.com/1099767

Would also make it easier to leverage existing libraries, such as $.proxy instead of __bind, as all helpers would be in one place.

@ryanflorence
Copy link

This is going a strange direction. Let's keep it simple. Add the flag, provide a file of all helpers.

On Nov 5, 2011, at 12:06 PM, Tasveer [email protected] wrote:

Awesome. This would definitely help with stuff like this https://gist.github.com/1099767

Would also make it easier to leverage existing libraries, such as $.proxy instead of __bind, as all helpers would be in one place.


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

@tazsingh
Copy link

tazsingh commented Nov 5, 2011

@rpflorence Duh. Maybe I wasn't clear enough.

I was commenting on the added benefit of being able to modify that single file of all helpers to further benefit my app's requirements.

Instead of this in all of my files, for example:

some_func = $.proxy(->
  # ...
, @)

I can use:

some_func = =>
  # ...

And overwrite my utilities.js to do the $.proxy behind the scenes. If you look at my gist, it would be useful to overwrite __extends as well to use Underscore's extend, and so on. Once again, all for my single application in just my utilities.js file that is provided by CoffeeScript.

Anywho, that's my 2 cents. Keep up the good work guys.

@TrevorBurnham
Copy link
Collaborator Author

I believe @rpflorence was referring to the namespacing suggestion.

On Nov 5, 2011, at 3:13 PM, Tasveer [email protected] wrote:

@rpflorence Duh. Maybe I wasn't clear enough.

I was commenting on the added benefit of being able to modify that single file of all helpers to further benefit my app's requirements.

Instead of this in all of my files, for example:

some_func = $.proxy(->
# ...
, @)

I can use:

some_func = =>
# ...

And overwrite my utilities.js to do the $.proxy behind the scenes. If you look at my gist, it would be useful to overwrite __extends as well to use Underscore's extend, and so on. Once again, all for my single application in just my utilities.js file that is provided by CoffeeScript.

Anywho, that's my 2 cents. Keep up the good work guys.


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

@tazsingh
Copy link

tazsingh commented Nov 5, 2011

@TrevorBurnham @rpflorence Ah. Seems we could've all been a bit clearer :P I was referring to $ as jQuery. I've had no conflicts with __ thus far, and would prefer to keep it simple and leave it the way it is.

@michaelficarra
Copy link
Collaborator

+1 this pull request as is. I can't believe how fast the second half of this discussion went completely off course.

@erisdev
Copy link

erisdev commented Nov 9, 2011

Yeah, 👍 as is. I think derailing the discussion was my fault. C:

@omarkhan
Copy link

omarkhan commented Nov 9, 2011

👍 to this!

@ryanflorence
Copy link

👍

@erisdev
Copy link

erisdev commented Nov 15, 2011

@TrevorBurnham …and pretty much every single other useful array method.

@jashkenas If you don't like the idea of a flag to exclude the helpers, would you consider some way of separating "boilerplate" code from code that corresponds directly to CoffeeScript source? There doesn't seem to be a good way of identifying and removing the CoffeeScript helpers otherwise.

@@ -117,6 +116,12 @@ task 'build:browser', 'rebuild the merged script for inclusion in the browser',
console.log "built ... running browser tests:"
invoke 'test:browser'

task 'build:utilities', 'rebuild the utilities file for use with compile(utilities: false)', ->
code = 'var ' + (for utility, func of require('./lib/coffee-script/nodes.js').UTILITIES
"__#{utility} = #{func()}"
Copy link

Choose a reason for hiding this comment

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

Not work for me : (

$ cake build:utilities

node.js:201
throw e; // process.nextTick error, or 'error' event on first tick
^
TypeError: Cannot read property 'utilities' of null
at ./coffee-script/lib/coffee-script/nodes.js:2472:19
at ./coffee-script/lib/coffee-script/nodes.js:2429:76
at ./coffee-script/Cakefile:138:49
at Object.action (./coffee-script/Cakefile:141:6)
at /lib/node_modules/coffee-script/lib/coffee-script/cake.js:39:26
at Object.run (/lib/node_modules/coffee-script/lib/coffee-script/cake.js:58:21)
at Object. (/lib/node_modules/coffee-script/bin/cake:7:38)
at Module._compile (module.js:432:26)
at Object..js (module.js:450:10)
at Module.load (module.js:351:31)

@erisdev
Copy link

erisdev commented Feb 21, 2012

I'm a little disappointed that the discussion here has stagnated. Can we get a conversation going again? This issue has raised its head again in a situation where I'd like to combine the compiled javascript output in a way that isn't possible with --join.

My use case right now is similar to the Compiled Modules example on the CommonJS wiki. I'd like to distribute my source code without depending on (or worse, bundling) a custom build of CoffeeScript.

@ryanflorence
Copy link

I think its stagnated because gzip, for the most part, makes the duplicated boilerplate's impact negligible.

@erisdev
Copy link

erisdev commented Feb 22, 2012

That's unfortunate, especially since the fix has already been implemented. And it makes the output uglier than it really needs to be. 💩

There was also the earlier suggestion that, with the option to provide our own utility functions, we could make CoffeeScript use those provided by another library. Not that it's massively important, but it's a nice bonus for no extra work.

@christophercurrie
Copy link

A big +1 for this. I think @TrevorBurnham has the right idea, this change would appear to make it possible (easy, even?) for require-cs to add support for this when run under the RequireJS optimizer.

@airhorns
Copy link

I would very much like this to be merged as well. The situation as it is effectively discourages file-heavy project layouts as you pay a price for every new file you make. If I want to make a client side application and I adopt a Rails-esque project structure where I have one class per file, I feel dirty every time I make a new file because I'm wasting bytes. As it stands right now CoffeeScript helpers take up 3% of my project's 1.1 MB of JS.

@michaelficarra michaelficarra mentioned this pull request Apr 8, 2012
@jashkenas
Copy link
Owner

I'm afraid I think that this pull request is still incomplete -- the proposed generated utilities.js script here isn't cross-runtime compatible. I think if we merged this now, and folks started using it, we'd just break them later on when we fix this patch.

Instead, for the time being, I think it's wiser for a file-heavy project to join their CoffeeScripts before compiling ... and if that's not an option -- just change your build script to nuke the helpers and replace them with your own references.

@jashkenas jashkenas closed this Apr 10, 2012
@TrevorBurnham
Copy link
Collaborator Author

the proposed generated utilities.js script here isn't cross-runtime compatible.

How so?

@jashkenas
Copy link
Owner

Naked variables without var aren't the way to export functions cross-module in Node.

@TrevorBurnham
Copy link
Collaborator Author

But Nodists aren't concerned about minimizing code size, which is the intent of the flag. I don't see how the flag could negatively affect them. Libraries that aim to run in both environments have two separate build processes, one that minimizes and concatenates everything and one that doesn't. Only the browser-targeted build would make use of this flag.

@jashkenas
Copy link
Owner

Alright -- aside from the minor var quibble, I'm still uncomfortable having the compiler generate broken code that you have to use a script to fix yourself by hand. You might as well just use a script in the first place (or, easier, just compile together). Leaving closed.

@TrevorBurnham
Copy link
Collaborator Author

OK, but weigh this approach against the alternatives:

  1. "...it's wiser for a file-heavy project to join their CoffeeScripts before compiling." I'm very surprised to hear you say this. I thought the function wrapper was one of the Good Parts of CoffeeScript, even for modules within a project.
  2. "...change your build script to nuke the helpers and replace them with your own references." Doable, but a lot more work than doing it on our end—work that will have to be replicated for each build environment (Node, Ruby/Rails, etc.) and updated for each version of CoffeeScript.

This is one of the features most commonly requested by people who are using CoffeeScript in production. I understand that this patch feels like a kludge, but given that helpers are here to stay and that the CoffeeScript compiler isn't aware of the target environment, isn't this the only way for us to provide that feature?

@jashkenas
Copy link
Owner

...it's wiser for a file-heavy project to join their CoffeeScripts before compiling.

The function wrapper is a good part of CoffeeScript to prevent global variables ... not to hide parts of your program from one another. Ideally, it's all working together in harmony in any case, so that even if it works if you don't --join the whole thing ... it'll also work if you do.

I understand that this patch feels like a kludge, but given that helpers are here to stay ...

Better not to commit kludges, even temporarily. They tend to become permanent, or impossible to remove. I'm definitely open to patches to relieve pressure from duplicated helpers, but we should do it "right" (whatever that means).

@michaelficarra
Copy link
Collaborator

This is unfortunate :(. Build tools really need this interface.

@delaaxe
Copy link

delaaxe commented Aug 13, 2012

nuke the helpers and replace them with your own references

Did anyone come up with a script for this?

@lydell
Copy link
Collaborator

lydell commented Nov 21, 2013

As this comes up every now and then, and people get directed here:

I've done some testing on this. With gzip the helper functions make almost no difference; In my specific test only 200-300 bytes.

@samuelhorwitz
Copy link

@lydell the size isn't necessarily the issue. With code coverage reporters, for example, you end up with branches that are marked as uncovered despite the Coffeescript not showing any branches. You can see the hack I have in place to solve this here Constellation/ibrik#36

This isn't only about splitting hairs about file size.

@DinisCruz
Copy link

@lydell I'm having the same problem has @samuelhorwitz , the inclusion of these extra methods (like the check for indexOf) it creating code paths that my code coverage checks cant reach

image

@lydell
Copy link
Collaborator

lydell commented Aug 10, 2015

@samuelhorwitz @DinisCruz The use case of this PR (as well as in a few other issues) is to reduce repetition of helper declarations, in order to save bytes when sending compiled JS from server to client. However, my tests have proven that this is unnecessary thanks to gzip.

Thanks for bringing up the issue with code coverage tools, though, because I've not heard and thought about such issues before.

I don't see how this PR would help in the code coverage case, though. Even if the helpers are not duplicated, there can still be unused branches is them. Removing all helpers won't cut it either, since then your program won't run at all -- unless you were planning to inject custom versions of them on your own? Feels pretty hacky.

It sounds like you'd rather be looking for some way to extend or override the helpers, more like what is done in the PR @samuelhorwitz linked to. Perhaps we could add an option to pass in a function that would pre-process the helper strings (that'd be done on this line). Then you could do regex hacks on it, or parse it using one of the many JavaScript parsers, modify the AST and return in a string using one of the many AST-to-string modules. For example, you could iterate over all statements and add skip comments to them.

@jashkenas
Copy link
Owner

I'd tend to think that points to a fault in the capability of the code coverage tool — better to fix it at the source, instead of contorting your code, no?

@samuelhorwitz
Copy link

Yeah, that's what this beautiful PR I submitted to the Ibrik team does :P https://github.com/Constellation/ibrik/pull/36/files

I'm not familiar enough with the Coffeescript compiler to know what the best way to go about doing what I'm trying to do is, but I'm pretty sure my solution isn't it, though it works.

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.