Support for license comments /! license / #332

kennethkufluk · 2012-03-02T21:44:29Z

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.

/*! 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.

RGustBardon · 2012-03-04T13:52:10Z

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 · 2012-03-05T02:09:33Z

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

fat · 2012-03-05T02:21:18Z

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 · 2012-03-05T03:00:16Z

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 · 2012-03-15T13:04:23Z

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.

cowboy · 2012-03-23T12:08:14Z

A huge +1 for this functionality being added in!

quaelin · 2012-03-29T20:29:53Z

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

xrstf · 2012-04-06T21:36:43Z

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 · 2012-04-21T23:00:06Z

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 · 2012-04-21T23:47:36Z

@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 · 2012-04-22T00:06:44Z

@mal Hmm, good catch.

mal · 2012-04-22T00:32:08Z

@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 · 2012-04-22T00:41:20Z

@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 · 2012-04-22T01:34:25Z

@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 · 2012-04-22T01:47:55Z

@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 · 2012-04-22T02:45:11Z

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

mal · 2012-04-22T14:59:15Z

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 · 2012-04-22T18:00:26Z

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 · 2012-04-22T18:08:41Z

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 · 2012-04-22T20:05:35Z

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 · 2012-04-22T20:28:22Z

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

cowboy · 2012-04-23T02:38:52Z

@mal fair enough!

apendua · 2015-03-21T10:28:21Z

@mal In "compiled" version of [email protected] 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 · 2015-03-21T10:30:32Z

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

mal · 2015-03-21T10:32:41Z

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

apendua · 2015-03-21T10:34:01Z

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

peterbe · 2016-01-11T18:21:53Z

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

Remove extra next() from multiline comments.

eba6613

Remove duplicate ./apachectl stop Remove comments.

ea7f505

jzaefferer reviewed Mar 15, 2012
View reviewed changes

kennethkufluk added 3 commits March 18, 2012 15:08

Tidy error messages in parser.

a2ba573

Add option -nl to remove license statements from generated code.

3321026

Add tests for license statement.

7551bf1

mal mentioned this pull request May 12, 2012

Preserve leading license comments from multiple files; //, /* and /*! #379

Closed

leoner mentioned this pull request Oct 8, 2012

注释问题 spmjs/spm#334

Closed

terinjokes mentioned this pull request Dec 14, 2012

Add "@license" to copyright header block lodash/lodash#138

Closed

shama mentioned this pull request Dec 31, 2012

Preserve /*! comments from being stripped gruntjs/grunt#596

Closed

pako-pl mentioned this pull request Apr 10, 2016

--comments option does not preserver jQuery license header by default mishoo/UglifyJS#1035

Closed

kennethkufluk closed this Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for license comments /! license / #332

Support for license comments /! license / #332

kennethkufluk commented Mar 2, 2012

RGustBardon commented Mar 4, 2012

kennethkufluk commented Mar 5, 2012

fat commented Mar 5, 2012

kennethkufluk commented Mar 5, 2012

jzaefferer Mar 15, 2012

kennethkufluk Mar 15, 2012

jzaefferer Mar 18, 2012

kennethkufluk Mar 18, 2012

cowboy commented Mar 23, 2012

quaelin commented Mar 29, 2012

xrstf commented Apr 6, 2012

mal commented Apr 21, 2012

cowboy commented Apr 21, 2012

kennethkufluk commented Apr 22, 2012

mal commented Apr 22, 2012

kennethkufluk commented Apr 22, 2012

mal commented Apr 22, 2012

kennethkufluk commented Apr 22, 2012

cowboy commented Apr 22, 2012

mal commented Apr 22, 2012

mal commented Apr 22, 2012

cowboy commented Apr 22, 2012

mathiasbynens commented Apr 22, 2012

mal commented Apr 22, 2012

cowboy commented Apr 23, 2012

apendua commented Mar 21, 2015

apendua commented Mar 21, 2015

mal commented Mar 21, 2015

apendua commented Mar 21, 2015

peterbe commented Jan 11, 2016

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

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

Conversation

kennethkufluk commented Mar 2, 2012

RGustBardon commented Mar 4, 2012

kennethkufluk commented Mar 5, 2012

fat commented Mar 5, 2012

kennethkufluk commented Mar 5, 2012

jzaefferer Mar 15, 2012

Choose a reason for hiding this comment

kennethkufluk Mar 15, 2012

Choose a reason for hiding this comment

jzaefferer Mar 18, 2012

Choose a reason for hiding this comment

kennethkufluk Mar 18, 2012

Choose a reason for hiding this comment

cowboy commented Mar 23, 2012

quaelin commented Mar 29, 2012

xrstf commented Apr 6, 2012

mal commented Apr 21, 2012

cowboy commented Apr 21, 2012

kennethkufluk commented Apr 22, 2012

mal commented Apr 22, 2012

kennethkufluk commented Apr 22, 2012

mal commented Apr 22, 2012

kennethkufluk commented Apr 22, 2012

cowboy commented Apr 22, 2012

mal commented Apr 22, 2012

mal commented Apr 22, 2012

cowboy commented Apr 22, 2012

mathiasbynens commented Apr 22, 2012

mal commented Apr 22, 2012

cowboy commented Apr 23, 2012

apendua commented Mar 21, 2015

apendua commented Mar 21, 2015

mal commented Mar 21, 2015

apendua commented Mar 21, 2015

peterbe commented Jan 11, 2016

Support for license comments /! license / #332

Support for license comments /! license / #332