Adding header to generated JS (#1778)

[TrevorBurnham]

The proposal at #1778 seemed to get a favorable reception. Here's an implementation. As discussed:

• The option is on the CoffeeScript.compile function. It's disabled by default.
• The header is added to all JS files generated with the coffee command (-c), but not to stdio output (e.g. -p).

Thoughts? Refinements?

[michaelficarra]

I think the version number should be the first 10 hex digits of the commit hash when their version isn't tagged. So, we shouldn't be marking things as 1.1.3-pre, only 1.1.3 or d359764fba. And it should include the date/time (localised) of compilation. Also, I don't think any of the files in our own /lib should be compiled with the headers. We don't want the headers changing with every commit.

[TrevorBurnham]

I think the version number should be the first 10 hex digits of the commit hash when their version isn't tagged.

That's a very interesting suggestion, and I've wanted something like that for a while, but I'm not sure that it can be implemented in any way that wouldn't be Rube Goldberg-esque. Consider:

You make a commit, which gets an ID. Now you want that ID to become part of the (committed) source. But if you do another commit, then that'll have a different ID. Even git commit --amend will change the commit ID.

So, maybe I'm missing some git-jitsu, but the only way I see that working is if every ordinary commit were followed by a separate commit that changed the version. It's possible (you could have a cake:build create a post-commit hook to ensure that all committers did this), but it'd mean that every second commit would be noise...

[michaelficarra]

@TrevorBurnham: I'm not talking about adding the commit identifier of the project (like, say, mauricemach/zappa), but of the coffeescript compiler itself. And only when the version is not tagged. This would only cause the problem you described for us, jashkenas/coffee-script, which is why I recommended we don't compile the files in our /lib with the headers so that they aren't changing with every commit.

[TrevorBurnham]

Right, I believe we're talking about the same thing. How would you go about making CoffeeScript.VERSION change on every commit?

On Oct 22, 2011, at 5:26 PM, Michael [email protected] wrote:

@TrevorBurnham: I'm not talking about adding the commit identifier of the project (like, say, zappa), but of the coffeescript compiler itself. And only when the version is not tagged. This would only cause the problem you described for us, jashkenas/coffee-script, which is why I recommended we don't compile the files in our /lib with the headers so that they aren't changing with every commit.

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

[michaelficarra]

I don't know what you mean. Why would CoffeeScript.VERSION change? That will be 1.1.3-pre or whatever. When putting out the header, it will output git log -n 1 | head -n 1 | cut -d \ -f 2 | cut -c 1-10 if the current version is not tagged (and only if it resides in a git repo, which the error code of git status will tell us).

[TrevorBurnham]

As satyr might put it:

$ npm install -g coffee-script
/usr/local/bin/coffee -> /usr/local/lib/node_modules/coffee-script/bin/coffee
$ cd /usr/local/lib/node_modules/coffee-script
$ git log -n 1
fatal: Not a git repository (or any of the parent directories): .git

(Actually, on my system, git commands do work from that directory—they just refer to Homebrew's git repo, not CoffeeScript's.)

Also, I don't see how

js = (require 'coffee-script').compile 'x=y', header: on

could possibly work if a git process had to be spawned somewhere before generating the header.

Or maybe I still don't understand what you're suggesting?

[michaelficarra]

@TrevorBurnham: You must have missed the part of my comment that says we only look at the git commit identifier if the error code of git status tells us that the compiler is in a git repo. This isn't that complicated of an idea. If you want, I can make a pull request of my own.

[TrevorBurnham]

@michaelficarra OK; I look forward to seeing your pull request, then. Incidentally, you might want to use git describe --tag --abbrev=10 rather than the git log command you described. Its output looks like this:

$ git checkout 1.1.2 && git describe --tag --abbrev=10
1.1.2
$ git checkout master && git describe --tag --abbrev=10
1.1.2-138-gd359764fba

where 138 is the number of commits since the tagged version. Very helpful, IMHO.

[michaelficarra]

@TrevorBurnham: I just tried implementing my idea, but got stuck because we have a synchronous interface and node only has an asynchronous child process handling interface. We need to have both sync and async interfaces like node does. So here's what I came up with:

diff --git a/src/coffee-script.coffee b/src/coffee-script.coffee
index df192a74..48ec2fd8 100644
--- a/src/coffee-script.coffee
+++ b/src/coffee-script.coffee
@@ -20,7 +20,8 @@ else if require.registerExtension
   require.registerExtension '.coffee', (content) -> compile content

 # The current CoffeeScript version number.
-exports.VERSION = '1.1.3-pre'
+VERSION = '1.1.3-pre'
+exports.VERSION = VERSION

 # Words that cannot be used as identifiers in CoffeeScript code
 exports.RESERVED = RESERVED
@@ -30,12 +31,44 @@ exports.helpers = require './helpers'

 # Compile a string of CoffeeScript code to JavaScript, using the Coffee/Jison
 # compiler.
-exports.compile = compile = (code, options = {}) ->
+exports.compile = compile = (code, options = {}, cb) ->
+  if typeof options is 'function' and !cb?
+    cb = options
+    options = {}
   try
-    (parser.parse lexer.tokenize code).compile options
+    js = (parser.parse lexer.tokenize code).compile options
   catch err
     err.message = "In #{options.filename}, #{err.message}" if options.filename
     throw err
+  return cb?(js) unless options.header
+  spawn = require('child_process').spawn
+  version = VERSION
+  date = (new Date).toUTCString()
+  genHeader = ->
+    header = "Generated by CoffeeScript #{version} on #{date}"
+    cb? "// #{header}\n#{js}"
+  (spawn 'which', ['git']).on 'exit', (whichGitCode) ->
+    return genHeader() unless whichGitCode is 0
+    (spawn 'git', ['status']).on 'exit', (gitStatusCode) ->
+      return genHeader() unless gitStatusCode is 0
+      (spawn 'git', ['describe', '--tag', '--abbrev=10']).stdout.on 'data', (gitVersion) ->
+        version = gitVersion
+        genHeader()
+
+exports.compileSync = compileSync = (code, options = {}) ->
+  try
+    js = (parser.parse lexer.tokenize code).compile options
+  catch err
+    err.message = "In #{options.filename}, #{err.message}" if options.filename
+    throw err
+  return js unless options.header
+  version = VERSION
+  # if node provided a synchronous child process API, we could do something like this:
+  #    if spawn('git status').code is 0
+  #     version = spawn('git describe --tag --abbrev=10').stdout
+  date    = (new Date).toUTCString()
+  header  = "Generated by CoffeeScript #{version} on #{date}"
+  "// #{header}\n#{js}"

I'll open an issue for adding an async API.

[TrevorBurnham]

@michaelficarra Is there a compelling use case for all this? You're proposing an expansion of the API just so that folks who use untagged versions to build their projects can see, from the top line of their JS files, exactly which commit was used to build them. Personally, I wouldn't have much use for this feature, because I only use untagged versions of CoffeeScript when participating in CoffeeScript issue discussions; when I'm building JS files, I'm using the latest release. So the header, as it exists in this pull request, is perfectly suited to my needs, and to the needs of anyone who only uses stable CoffeeScript versions for their projects.

Adding headers with more precise version information seems neat, but if it can't be done in a more elegant way than spawning a process that only works if CoffeeScript was installed in a certain way (and obviously won't work at all in a browser environment), then it would seem more appropriate to create a separate tool for the job.

[michaelficarra]

@TrevorBurnham: I'm not saying this is a compelling reason to change the API. I'm saying the API should be changed anyway, and this feature depends on it being changed. It wouldn't be a big deal if we had had an async API already or if node provided a synchronous child process interface.

edit: For now, I'd be okay with something like this:

exports.compile = compile = (code, options = {}) ->
  try
    js = (parser.parse lexer.tokenize code).compile options
  catch err
    err.message = "In #{options.filename}, #{err.message}" if options.filename
    throw err
  return js unless options.header
  version = VERSION
  date    = (new Date).toUTCString()
  header  = "Generated by CoffeeScript #{version} on #{date}"
  "// #{header}\n#{js}"

We obviously shouldn't wait on an API change to implement #1788.

[jashkenas]

Hold off on merging this for the time being, if you don't mind -- I'm still fairly wary of adding a header to every file. We're already running into situations where it's not desirable.

[TrevorBurnham]

Sure thing. Awaiting your approval.

We're already running into situations where it's not desirable.

Are you referring to the lib/coffee-script/*.js files getting the header? That struck me as a little odd at first too, but it could be useful as a sanity check, in case someone did a build using a different version of the compiler than they'd intended. Note that the one file in that directory that isn't compiled from a corresponding .coffee file is parser.js, which begins with the comment

/* Jison generated parser */

so it makes sense from a documentation standpoint that the other .js files would start with

// Generated by CoffeeScript 1.1.3-pre

Of course, the changing header would add some commit clutter, but that'd only happen when CoffeeScript.VERSION gets bumped.

[TrevorBurnham]

Returning to the original proposal: Have you had time to consider this, @jashkenas? Having

// Generated by CoffeeScript [version]

at the top of generated JS files would be helpful both to humans and to tools (like GitHub) that have good reason to treat generated JS differently than hand-coded JS.

[jashkenas]

I think it sounds good. Want to fix this pull req to merge cleanly?

[TrevorBurnham]

Done.

[jashkenas]

Uh, oh -- if you notice in the commit, this actually breaks Github's current auto-detection. Pinging @josh to take a look.

[michaelficarra]

@TrevorBurnham: consider using the code I posted above instead. That way, we get the date as well, and it's a little cleaner with the early exit.

[jashkenas]

I don't think we want the date -- the relevant thing should be the version of the compiler used.

[TrevorBurnham]

I'd suggest to @josh that GitHub should suppress diffs on all JS files starting with // Generated by (in addition to the kinds of diffs that are currently suppressed), so that other JS generators have a standard to follow.

@michaelficarra I agree with Jeremy that a date in the header is TMI. It'd result in a lot of unnecessary diffs, since coffee currently recompiles everything in the directory when it starts up, and gitignore-ing generated JS isn't always wise.

[michaelficarra]

@TrevorBurnham: I'm convinced. How about that early exist, though? It's a lot cleaner.

[josh]

A // Generated by convention sounds great. I'll add that to our detection if this is merged.

[jashkenas]

@TrevorBurnham: throw in the early return and feel free to merge. In general, let's never be reliant on the coercion of [undefined] into a string interpolation.

[TrevorBurnham]

Cool. Done and done.

[michaelficarra]

Awesome. This one has been a long time coming.