Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Support for license comments /*! license */ #332

Open
wants to merge 6 commits into from

9 participants

Kenneth Kufluk Robert Gust‑Bardon F A T Ben Alman Chris Campbell Christoph Mal Graty Mathias Bynens Jörn Zaefferer
Kenneth Kufluk

Adding support for license statements marked like this:
/*!
license
*/
These will be new elements in the AST, and will always be generated into the minified code.
This fixes #85, #306, some of #275, maybe some others.

I also pass the comments and line numbers out of the parser, by adding extra properties to the AST arrays.
This is useful to those using the parser without the minifier.

Kenneth Kufluk Support for licenses statements marked like this:
/*!
  license
*/
These will be new elements in the AST, and will always be generated into the minified code.

I also pass the comments and line numbers out of the parser, by adding extra properties to the AST arrays.
This is useful to those using the parser without the minifier.
28efb89
Robert Gust‑Bardon

Due to an extra next() (line 579 of ./lib/parse-js.js):

  • $ echo '/**/' | uglifyjs results in an error,
  • $ echo '/*a*/' | uglifyjs results in another error,
  • $ echo '/*ab*/' | uglifyjs results in /*b*/;.
Kenneth Kufluk

Hmm. Yes, good point. Wondered whether that next() would affect anything.

F A T
fat commented

This might break the bin implementation which already extracts comments and then readds them.

https://github.com/mishoo/UglifyJS/blob/master/bin/uglifyjs#L265

Kenneth Kufluk

I think it's compatible with show_copyight. Doesn't cause me errors with a few simple test cases.

(Sorry for weird last commit message. Forgot !s didn't work in commit msgs.)

lib/parse-js.js
@@ -258,6 +258,12 @@ function JS_Parse_Error(message, line, col, pos) {
this.col = col + 1;
this.pos = pos + 1;
this.stack = new Error().stack;
+ try {

What does this have to do with the pull request?

Nothing. It just reports errors with context, which makes it far far easier to debug this sort of thing.

Shouldn't you then remove line 260?

I've removed this, for a cleaner pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Ben Alman

A huge +1 for this functionality being added in!

Chris Campbell

Another +1... I'm using @kennethkufluk's fork in the meantime (thanks!).

Christoph

Another +1 -- this makes UglifyJS more suitable for people wanting to use the "official" (1) way to embed their license.

(1) jQuery does it, so it must be "the right way". ;-)

Mal Graty

Unfortunately adding the license to the AST means that it breaks contextual minifications that look around using prev() and peek(), this is especially apparent when using --no-licenses mode.

in.js

/*! License 1 */
/*! Multiline
    License 2
*/
var a=1;
/*! License 3 */
var b=1;

var a=1,b=1;

mishoo/master

$ uglifyjs -nc in.js
var a=1,b=1;

kennethkufluk/master

$ bin/uglifyjs -nc -nl in.js
var a=1;var b=1;

The new license node prevents the code processing the var b from seeing that the previous node was var a and combining the two.

Ben Alman

@mal that makes sense, and is the behavior I'd expect. The most common use-case for preserving inline license comments assumes they are at the top of individual files concatenated together into a single script, in which case each sub-script will most likely be inside an IIFE anyways.

Kenneth Kufluk

@mal Hmm, good catch.

Mal Graty

@cowboy I agree with the common use-case, however there's still some loss with IIFEs; consider:

/*! License 1 */
(function(){var a=1;})();
/*! License 2 */
(function(){var b=1;})();

mishoo/master

(function(){var a=1})(),function(){var a=1}();

kennethkufluk/master

(function(){var a=1})();(function(){var a=1})();
Kenneth Kufluk

@mal I still think the license should be in the AST, to prevent loss & confusion during mangling.

I guess I could add exceptions to the next, prev and peek methods? Do you think that would work?

Mal Graty

@kennethkufluk In theory ignoring non-code AST nodes unless processing the node itself should resolve that particular problem, but there's other perils associated with having non-code items as first class nodes in the AST:

Function Hoisting

/*! License goes walk about with no IIFE */
function a () {}
var b;
function a(){}/*! License goes walk about with no IIFE */var b;

Sequences

var a=1,
/*! Very suspiciously placed license comment kills the parser */
b=1;
DEBUG: Error
    at new JS_Parse_Error (/home/mal/code/js/UglifyJS/lib/parse-js.js:260:22)
    ...
Kenneth Kufluk

@mal Yes, I see the problem.

I always knew that badly placed licenses would cause havoc. You can put a comment pretty much anywhere. Maybe we need to exclude badly-placed licenses. But, hey, licenses are not hard to place.

Maybe the solution is to simply not AST them if --no-licenses is enabled, solving the issue for those that don't care.
And otherwise leave as-is - the licenses forming barriers between licensed blocks of code.

Ben Alman

FWIW, keeping /*! ... */ comments is great, but it would be nice if there was also an option to keep all /* ... */ comments.

Mal Graty

In an ideal world I think we'd just keep the comments_before property while the AST gets manipulated, and then either output as many or as few comments as we like from gen_code. The problem is that it's easier said than done; all node properties get dropped almost every time the tree is walked. I think it's the scale of the refactor required that has made it more of a 2.0 feature in past discussions.

For now though @kennethkufluk's suggestion of ignoring weird license placements sounds like the best short term solution. Potential rules being something like: they must be in the toplevel immediately preceding an IIFE or function?

Mal Graty

Alternatively; another, much simpler, solution that I imagine solves most use-cases (command line only) is to add documentation for, and make use of, the --make argument. It allows a JSON file to specify a set of files to be uglified, and uglifies them independently before concatenating the output. This means that the first comment in each file is preserved in place.

Ben Alman

Adding complex rules that dictate where to-be-preserved comments need to be located might be overly complex. What if you just documented that comment nodes between statements that would otherwise be combined will prevent them from being combined?

Mathias Bynens

RT @cowboy

Adding complex rules that dictate where to-be-preserved comments need to be located might be overly complex. What if you just documented that comment nodes between statements that would otherwise be combined will prevent them from being combined?

Mal Graty

@cowboy Agreed, documentation > complex rules, but at a minimum weirdly placed comments should not be allowed to crash the parser.

Ben Alman

@mal fair enough!

hui leoner referenced this pull request in spmjs/spm
Closed

注释问题 #334

Terin Stock terinjokes referenced this pull request in lodash/lodash
Closed

Add "@license" to copyright header block #138

Kyle Robinson Young shama referenced this pull request in gruntjs/grunt
Closed

Preserve /*! comments from being stripped #596

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 3, 2012
  1. Support for licenses statements marked like this:

    Kenneth Kufluk authored
    /*!
      license
    */
    These will be new elements in the AST, and will always be generated into the minified code.
    
    I also pass the comments and line numbers out of the parser, by adding extra properties to the AST arrays.
    This is useful to those using the parser without the minifier.
Commits on Mar 5, 2012
  1. Kenneth Kufluk
  2. Kenneth Kufluk
Commits on Mar 19, 2012
  1. Kenneth Kufluk
  2. Kenneth Kufluk
  3. Kenneth Kufluk
This page is out of date. Refresh to see the latest.
5 README.html
View
@@ -449,6 +449,11 @@ <h3 id="sec-1-4"><span class="section-number-3">1.4</span> Usage </h3>
etc.). If you pass this it will discard it.
</li>
+<li><code>-nl</code> or <code>--no-licenses</code> &mdash; by default, <code>uglifyjs</code> will keep the text
+ marked /*! */ in the generated code (assumed to be license information
+ etc.). If you pass this it will discard it.
+
+</li>
<li><code>-o filename</code> or <code>--output filename</code> &mdash; put the result in <code>filename</code>. If
this isn't given, the result goes to standard output (or see next one).
4 README.org
View
@@ -224,6 +224,10 @@ Supported options:
comment tokens in the generated code (assumed to be copyright information
etc.). If you pass this it will discard it.
+- =-nl= or =--no-licenses= --- by default, =uglifyjs= will keep the text
+ marked /*! */ in the generated code (assumed to be license information
+ etc.). If you pass this it will discard it.
+
- =-o filename= or =--output filename= --- put the result in =filename=. If
this isn't given, the result goes to standard output (or see next one).
9 bin/uglifyjs
View
@@ -3,7 +3,7 @@
global.sys = require(/^v0\.[012]/.test(process.version) ? "sys" : "util");
var fs = require("fs");
-var uglify = require("uglify-js"), // symlink ~/.node_libraries/uglify-js.js to ../uglify-js.js
+var uglify = require("../uglify-js"), // symlink ~/.node_libraries/uglify-js.js to ../uglify-js.js
consolidator = uglify.consolidator,
jsp = uglify.parser,
pro = uglify.uglify;
@@ -32,7 +32,8 @@ var options = {
indent_start: 0,
quote_keys: false,
space_colon: false,
- inline_script: false
+ inline_script: false,
+ licenses: true
},
make: false,
output: true // stdout
@@ -86,6 +87,10 @@ out: while (args.length > 0) {
case "-nc":
options.show_copyright = false;
break;
+ case "--no-licenses":
+ case "-nl":
+ options.codegen_options.licenses = false;
+ break;
case "-o":
case "--output":
options.output = args.shift();
37 lib/parse-js.js
View
@@ -466,7 +466,6 @@ function tokenizer($TEXT) {
};
function read_multiline_comment() {
- next();
return with_eof_error("Unterminated multiline comment", function(){
var i = find("*/", true),
text = S.text.substring(S.pos, i);
@@ -546,6 +545,18 @@ function tokenizer($TEXT) {
return token("operator", grow(prefix || next()));
};
+ function read_license() {
+ return with_eof_error("Unterminated license statement", function(){
+ var i = find("*/", true),
+ text = S.text.substring(S.pos, i);
+ for (var j=0; j<text.length+2; j++) {
+ next();
+ }
+ S.newline_before = text.indexOf("\n") >= 0;
+ return token("license", text);
+ });
+ };
+
function handle_slash() {
next();
var regex_allowed = S.regex_allowed;
@@ -555,8 +566,15 @@ function tokenizer($TEXT) {
S.regex_allowed = regex_allowed;
return next_token();
case "*":
- S.comments_before.push(read_multiline_comment());
+ next();
S.regex_allowed = regex_allowed;
+ if (peek()=='!') {
+ // A license statment starts /*!
+ next();
+ return read_license();
+ } else {
+ S.comments_before.push(read_multiline_comment());
+ }
return next_token();
}
return S.regex_allowed ? read_regexp("") : read_operator("/");
@@ -682,7 +700,6 @@ function NodeWithToken(str, start, end) {
NodeWithToken.prototype.toString = function() { return this.name; };
function parse($TEXT, exigent_mode, embed_tokens) {
-
var S = {
input : typeof $TEXT == "string" ? tokenizer($TEXT, true) : $TEXT,
token : null,
@@ -755,7 +772,12 @@ function parse($TEXT, exigent_mode, embed_tokens) {
};
function as() {
- return slice(arguments);
+ var arr = slice(arguments);
+ if (S.token.comments_before && S.token.comments_before.length) {
+ arr.comments_before = S.token.comments_before;
+ }
+ if (S.token.line) arr.line = S.token.line;
+ return arr;
};
function parenthesised() {
@@ -792,6 +814,9 @@ function parse($TEXT, exigent_mode, embed_tokens) {
case "atom":
return simple_statement();
+ case "license":
+ return as("license", prog1(S.token.value, next));
+
case "name":
return is_token(peek(), "punc", ":")
? labeled_statement(prog1(S.token.value, next, next))
@@ -886,6 +911,10 @@ function parse($TEXT, exigent_mode, embed_tokens) {
return as("label", label, stat);
};
+ function license() {
+ return as("license", prog1(expression));
+ };
+
function simple_statement() {
return as("stat", prog1(expression, semicolon));
};
12 lib/process.js
View
@@ -202,6 +202,9 @@ function ast_walker() {
},
"atom": function(name) {
return [ this[0], name ];
+ },
+ "license": function(name) {
+ return [ this[0], name ];
}
};
@@ -1418,7 +1421,8 @@ function gen_code(ast, options) {
space_colon : false,
beautify : false,
ascii_only : false,
- inline_script: false
+ inline_script: false,
+ licenses: true
});
var beautify = !!options.beautify;
var indentation = 0,
@@ -1547,6 +1551,12 @@ function gen_code(ast, options) {
"string": encode_string,
"num": make_num,
"name": make_name,
+ "license": function(text){
+ if (!options.licenses) {
+ return '';
+ }
+ return "/*!" + text + '*/';
+ },
"debugger": function(){ return "debugger" },
"toplevel": function(statements) {
return make_block_statements(statements)
1  test/testparser.js
View
@@ -23,6 +23,7 @@ function ParserTestSuite(callback){
["var abc = 5;", "Regular variable statement with assignment"],
["/* */;", "Multiline comment"],
['/** **/;', 'Double star multiline comment'],
+ ["/*! */;", "License statement"],
["var f = function(){;};", "Function expression in var assignment"],
['hi; // moo\n;', 'single line comment'],
['var varwithfunction;', 'Dont match keywords as substrings'], // difference between `var withsomevar` and `"str"` (local search and lits)
3  test/unit/compress/expected/license.js
View
@@ -0,0 +1,3 @@
+/*! License 1 *//*! Multiline
+ License 2
+*/var a=1;/*! License 3 */var b=1
7 test/unit/compress/test/license.js
View
@@ -0,0 +1,7 @@
+/*! License 1 */
+/*! Multiline
+ License 2
+*/
+var a=1;
+/*! License 3 */
+var b=1;
Something went wrong with that request. Please try again.