Skip to content

Fix shebang and header ordering #1032

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

Merged
merged 1 commit into from
Apr 12, 2018
Merged

Conversation

rhendric
Copy link
Collaborator

@rhendric rhendric commented Mar 27, 2018

Earlier support for shebang lines in the compiled output moved them before the default function wrapper, but not before the default LiveScript header comment. This moves shebangs above the header comment as well.

Fixes #1030.


#1030 seems like a reasonable request to me, but I might be missing some reason not to do this—speak up here if you can think of any. (There's some prior discussion at #497, which mostly just rejects new explicit compiler options in favor of the existing workarounds, but nobody proposed what #1030 proposes.) I'll hold this PR for two weeks, merging on or after April 10 if there are no objections.

Edit: It looks like #1030 as written isn't a good idea, so this PR now targets this comment.

@rhendric
Copy link
Collaborator Author

(One possible reason to object: shebangs ought to refer to a file's actual contents, not the contents of a derived file. #!/usr/bin/env node is technically incorrect for LS files. Conversely, an LS script can correctly use #!/usr/bin/env lsc and be executable, but with this change if that file were also to be compiled, the output would have an incorrect shebang.)

@pepkin88
Copy link
Contributor

For files intended to run with lsc, shebang should be:

#!/usr/bin/env lsc

But for files intended to be compiled with lsc and then run with node, the shebang line should be:

``#!/usr/bin/env node``

just like in example from #1030.
Why? Because it doesn't introduce any new syntax or exception to a rule.
Moreover, there wouldn't be incorrect shebangs left in files. The first example would strip the comment with an lsc shebang from the compiled file (correct), in the second example ``#! shouldn't be interpreted as a shebang (correct).

Flipping lines can stay, but I would rather test the compiled code for starting from #!.

@rhendric
Copy link
Collaborator Author

@nazar-pc, what do you think about that? If

``#!/usr/bin/env node``
console.log 'Hello'

compiled by default to

#!/usr/bin/env node
// Generated by LiveScript 1.5.0
(function(){
  console.log('Hello');
}).call(this);

would that look good to you?

@nazar-pc
Copy link

Having both support for running original script with LiveScript and compiled with Node directly seems highly unlikely, but still possible.

So if I understand correctly

#!/usr/bin/env lsc
``#!/usr/bin/env node``
console.log 'Hello'

would be still compiled by default to

#!/usr/bin/env node
// Generated by LiveScript 1.5.0
(function(){
  console.log('Hello');
}).call(this);

and work properly all the time.

I'm fine with that, makes perfect sense.

@rhendric rhendric force-pushed the fix/LiveScript-1030 branch from f6f9c34 to 9de5704 Compare March 28, 2018 20:43
Earlier support for shebang lines in the compiled output moved them
before the default function wrapper, but not before the default
LiveScript header comment. This moves shebangs above the header comment
as well.
@rhendric rhendric force-pushed the fix/LiveScript-1030 branch from 9de5704 to ceec281 Compare March 28, 2018 20:44
@rhendric rhendric changed the title Support shebang lines Fix shebang and header ordering Mar 28, 2018
@rhendric rhendric merged commit 41fb1a8 into gkz:master Apr 12, 2018
@nazar-pc
Copy link

Found minor issue with this:

/**
 * Whatever
 */

is compiled to

/**
 * Whatever
 */
// Generated by LiveScript 1.5.0

instead of previous

// Generated by LiveScript 1.5.0
/**
 * Whatever
 */

GitHub seems to use this header for identifying whether *.js file is standalone or was compiled from other language like LiveScript and starts showing incorrect distribution of languages used by project.

Would you be so kind to fix this and add one more test case?

@rhendric
Copy link
Collaborator Author

I'll look into it, thanks!

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.

3 participants