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

Open
wants to merge 6 commits into
from

Projects

None yet
@kennethkufluk

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
@RGustBardon
Contributor

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*/;.
@kennethkufluk

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

@fat
fat commented Mar 5, 2012

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

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

@kennethkufluk

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.)

@jzaefferer jzaefferer and 1 other commented on an outdated diff Mar 15, 2012
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 {
@jzaefferer
jzaefferer Mar 15, 2012

What does this have to do with the pull request?

@kennethkufluk
kennethkufluk Mar 15, 2012

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

@jzaefferer
jzaefferer Mar 18, 2012

Shouldn't you then remove line 260?

@kennethkufluk
kennethkufluk Mar 18, 2012

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

@cowboy
cowboy commented Mar 23, 2012

A huge +1 for this functionality being added in!

@quaelin
quaelin commented Mar 29, 2012

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

@xrstf
xrstf commented Apr 6, 2012

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
Contributor
mal commented Apr 21, 2012

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.

@cowboy
cowboy commented Apr 21, 2012

@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.

@kennethkufluk

@mal Hmm, good catch.

@mal
Contributor
mal commented Apr 22, 2012

@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})();
@kennethkufluk

@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
Contributor
mal commented Apr 22, 2012

@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)
    ...
@kennethkufluk

@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.

@cowboy
cowboy commented Apr 22, 2012

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

@mal
Contributor
mal commented Apr 22, 2012

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
Contributor
mal commented Apr 22, 2012

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.

@cowboy
cowboy commented Apr 22, 2012

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?

@mathiasbynens
Contributor

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
Contributor
mal commented Apr 22, 2012

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

@cowboy
cowboy commented Apr 23, 2012

@mal fair enough!

@leoner leoner referenced this pull request in spmjs/spm Oct 8, 2012
Closed

注释问题 #334

@terinjokes terinjokes referenced this pull request in lodash/lodash Dec 14, 2012
Closed

Add "@license" to copyright header block #138

@shama shama referenced this pull request in gruntjs/grunt Dec 31, 2012
Closed

Preserve /*! comments from being stripped #596

@apendua
apendua commented Mar 21, 2015

@mal In "compiled" version of jquery@2.0.3 they did this:

var Sizzle =
/*!
 * Sizzle CSS Selector Engine v2.2.0-pre
 * http://sizzlejs.com/
 *
 * Copyright 2008, 2014 jQuery Foundation, Inc. and other contributors
 * Released under the MIT license
 * http://jquery.org/license
 *
 * Date: 2014-12-16
 */
(function( window ) {
  // ...
});

At the same time, yuglify tried to hack uglify-js to prevent removing those comments. It all resulted in this funny problem

yui/yuglify#19

Well, to be honest I don't think preserving comments can be done in a reasonable way without some insight into ast. I believe that doing it outside uglify-js will always produce some unexpected edge cases.

It's been three years. Do you guys plan to merge this PR someday?

@apendua
apendua commented Mar 21, 2015

@mal The examples you provided are not going to happen in real life scenarios. Really.

@mal
Contributor
mal commented Mar 21, 2015

This entire project has been superceeded by https://github.com/mishoo/UglifyJS2 ...

@apendua
apendua commented Mar 21, 2015

@mal Ah, ok ... thank you for the information!

@peterbe
peterbe commented Jan 11, 2016

Yeah, I got stuck here too. Simply switching to uglify-js (npm package name for UglifyJS2) solved the problem I had (...in django-pipeline).

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