Proposal: Generate wrapper only when needed

[TrevorBurnham]

Lately, Google Dart has been getting some well-deserved flack for a trivial "Hello, world" example generating thousands of lines of JS output. Meanwhile, though, CoffeeScript is generating 3 lines when it could be generating just 1. Since

 console.log 'Hello, world!'

contains no scoped variables, I don't see any benefit to generating the output

(function() {
  console.log('Hello, world!');
}).call(this);

instead of just

console.log('Hello, world!');

I propose that the wrapper only be added when it has an effect on scope. What do other folks think?

[davidchambers]

I would love to see this. A common pattern is...

jQuery ($) ->
  # rest of file resides inside this function

When manually compiling a single file it's easy enough to use --bare. When writing a build script, though, or working on a team with people less familiar with CoffeeScript, compiling files differently based on their contents is problematic. On my team we've abandoned --bare to minimize friction. I'd love to see the top-level function wrapper omitted in cases where it's unnecessary (irrespective of --bare).

+1

[jashkenas]

I agree -- this would be rad.

[michaelficarra]

I love it. +1.

[paulmillr]

:+1:, great idea.

[satyr]

Watch also for top level returns which were guaranteed to work without --bare.

[michaelficarra]

@satyr: See #1319. I'd prefer they're disallowed entirely. A top-level return makes no sense, even if it is useful for control flow.

[showell]

FYI there will be a small doc fix needed, as it would no longer true that "all CoffeeScript output is wrapped in an anonymous function: (function(){ ... })();".

I'm in the minority here, but I don't understand why this fix is unnecessary. It seems to fix a non-problem. Google Dart is getting flack for a 17,000-line hello-world. CS has a perfectly reasonable 3-line hello-world that's probably already documented several places out in the wild. The safety wrapper is needed in 95% of cases, and in the 5% of cases where it could be optimized away, it's almost always in example code, and it's never an actual bottleneck.

The complexity cost is pretty small here, so I'm -0.001.

[showell]

P.S. While the safety wrapper seems like a misfeature for hello-world, it makes it crystal clear that the author intended the code to be safe. I'm not sure if humans care, but it's possible that asset management systems use this as a cue. It's probably moot in most cases.

[showell]

FYI Github uses the safety wrapper as a cue to suppress .JS files in diffs.

https://github.com/github/linguist/blob/master/lib/linguist/blob_helper.rb

    # Internal: Is the blob JS generated by CoffeeScript?
    #
    # Requires Blob#data
    #
    # CoffeScript is meant to output JS that would be difficult to
    # tell if it was generated or not. Look for a number of patterns
    # outputed by the CS compiler.
    #
    # Return true or false
    def generated_coffeescript?
      return unless extname == '.js'

      if lines[0] == '(function() {' &&     # First line is module closure opening
          lines[-2] == '}).call(this);' &&  # Second to last line closes module closure
          lines[-1] == ''                   # Last line is blank

        score = 0

        lines.each do |line|
          if line =~ /var /
            # Underscored temp vars are likely to be Coffee
            score += 1 * line.gsub(/(_fn|_i|_len|_ref|_results)/).count

            # bind and extend functions are very Coffee specific
            score += 3 * line.gsub(/(__bind|__extends|__hasProp|__indexOf|__slice)/).count
          end
        end

        # Require a score of 3. This is fairly arbitrary. Consider
        # tweaking later.
        score >= 3
      else
        false
      end
    end

[yuchi]

Good catch! Probably that section of the blob_helper should be modified to simply add some big score if the safety wrapper is found, but it should not require it anymore.

[showell]

@yuchi Even with modifications, the blob_helper code would have trouble distinguishing whether small JS files were hand-written or CS-generated, once the safety wrapper was removed. This is just a nuisance, of course, but the safety wrapper itself is nothing more than a nuisance as well.

[jashkenas]

Ping @josh, so he's aware of this potential change.

[josh]

@showell Right, these simple cases with or without the wrapper have always been too hard to detect. I'm not really worried about it. But if you have any improvements, please submit a pull. That's why it's open source ;)

Overall this change is pretty neat.

[TrevorBurnham]

It'd be trivial to detect a .js file as compiled from CoffeeScript if the compiler added a comment at the top to that effect (which would also have the notable advantage of aiding maintainability). I think this has been suggested before, but I can't find an issue, so, opening discussion at #1778.

[josh]

Heh, I suggested that too, but @jashkenas shot me down.

[showell]

Just to be clear, I'm merely arguing for the status quo here. The github example was only intended to show that this seemingly innocent fix might have unforeseen consequences. The prior boilerplate paradigm was consistent and easy to understand, even if it didn't optimize every trivial case.

[davidchambers]

It's not just for trivial cases, @showell. I have dozens of files which begin jQuery ($) ->. This change perfectly aligns with one of CoffeeScript's stated goals: to produce readable output.

[showell]

@davidchambers, I hate boilerplate code just as much as the next person, but removing the function wrapper actually harms readability IMHO. We're comparing...

jQuery ($) ->
    JS(cruft);
    JS(cruft);
    JS(cruft);

to...

BEGIN FAMILIAR, COMFY CS SAFETY WRAPPER
  jQuery ($) ->
    JS(cruft);
    JS(cruft);
    JS(cruft);
END FAMILIAR, COMFY CS SAFETY WRAPPER

The former is certainly more terse (by all of two lines), but the latter removes all doubts about side effects, which is one of the first thing code readers look for ("Can I treat this code as safe? Ok, I'll move on.").

The CS boilerplate only affects the first and last lines of a file, apart from indentation.

I'd be curious to see a real world example where this patch actually makes an impact and actually makes a difference in the CoffeeScript vs. Dart comparison.

[TrevorBurnham]

@showell I think David was speaking to the benefits of reduced byte size. Sure, it's just a dozen or so extra bytes per file, but across dozens of files, that's a significant boost in efficiency.

[showell]

@TrevorBurnham The performance impact of safety wrappers is completely negligible.

[TrevorBurnham]

@showell And yet I've seen folks asking for a way of turning on --bare on a file-by-file basis for just that reason. Some folks like to divide their front-ends into lots of small files.

[showell]

@TrevorBurnham There are plenty of legitimate reasons to use --bare. Sometimes you want global scope. But doing it for performance reasons would be plain moronic.

[showell]

Also, people who "need" the --bare option already have the --bare option.

[TrevorBurnham]

For the record: This feature was removed by e4b3e838e2f308d5d39eb54361374fb171ef58c9, between 1.1.3 and 1.2.0. Quoting @jashkenas:

I'd be glad to put it back in, if we can get the formatting correct.

[michaelficarra]

Thanks for the reminder, @TrevorBurnham.