Skip to content
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

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

Closed
wants to merge 6 commits into from

Conversation

kennethkufluk
Copy link

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
Copy link
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
Copy link
Author

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

@fat
Copy link

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
Copy link
Author

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this have to do with the pull request?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you then remove line 260?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cowboy
Copy link

cowboy commented Mar 23, 2012

A huge +1 for this functionality being added in!

@quaelin
Copy link

quaelin commented Mar 29, 2012

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

@xrstf
Copy link

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
Copy link
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
Copy link

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
Copy link
Author

@mal Hmm, good catch.

@mal
Copy link
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
Copy link
Author

@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
Copy link
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
Copy link
Author

@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
Copy link

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
Copy link
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
Copy link
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
Copy link

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
Copy link
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
Copy link
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
Copy link

cowboy commented Apr 23, 2012

@mal fair enough!

@apendua
Copy link

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
Copy link

apendua commented Mar 21, 2015

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

@mal
Copy link
Contributor

mal commented Mar 21, 2015

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

@apendua
Copy link

apendua commented Mar 21, 2015

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

@peterbe
Copy link

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.