Skip to content
This repository has been archived by the owner on Jun 15, 2021. It is now read-only.

Licensing concerns #43

Closed
tbranyen opened this issue Jun 10, 2011 · 28 comments
Closed

Licensing concerns #43

tbranyen opened this issue Jun 10, 2011 · 28 comments

Comments

@tbranyen
Copy link
Contributor

I grabbed the latest version of Ender and used the build command with 4 packages:

ender build domready qwery bean jquery

Note: the license for jQuery is dual MIT/GPLv2.

The output I got was:

/*!
  * Ender: open module JavaScript framework
  * copyright Dustin Diaz & Jacob Thornton 2011 (@ded @fat)
  * https://ender.no.de
  * License MIT
  * Build: ender build domready qwery bean jquery
  */
<minified code>

The concern I have is that not only is the minifed file licensed entirely under Dustin Diaz and Jacob Thornton, but its also entirely now licensed under MIT. All the required comment headers containing licensing are also now stripped.

Are there any plans to correct how this build script outputs?

@ded
Copy link
Member

ded commented Jun 10, 2011

At one point I wanted to add and respective licenses of each module blabla. I'll figure out a sensible way for this.

@fat
Copy link
Contributor

fat commented Jun 26, 2011

I changed our license to read:

/*!
 * Ender: open module JavaScript framework
 * copyright Dustin Diaz & Jacob Thornton 2011 (@ded @fat)
 * https://ender.no.de
 * License MIT 
 * Module's individual licenses still apply
 * Build: ender build qwery bean
 */

We also now issue a warning when building minified files that alerts a consumer that UgliflyJS strips comments. In the future we may consider switching from uglify to closure -- but at the moment it seems to make the most sense for us, as it's the lightest/fastest minification tool.

@tbranyen
Copy link
Contributor Author

@fat I recently forked Ender again and noticed that does, in fact, have the ability baked right in to handle licensing comments. It even looked at cursory glance to do this by default? Any idea why the copyright code here https://github.com/ender-js/Ender/blob/master/lib/ender.file.js#L296 isn't being honored?

Is it because you are concatenating all the files and then running it through uglify-js instead of individually and concatenating the result?

@tbranyen
Copy link
Contributor Author

I still don't understand why you need your copyright in a file where it does not belong. To what code are you copyrighting? It feels wrong.

@ded
Copy link
Member

ded commented Jun 26, 2011

Uglify honors the first comment in a document.
Also, we're licensing Ender, the client. It is, of course, what makes everything work together.

@tbranyen
Copy link
Contributor Author

@ded oh I understand that completely and I agree ender need licensing in its source code, but to put that in a file that doesn't contain any ender code seems weird don't you agree? Unless I'm mistaken the minified build contains all third-party code.

In regards to your comment on the first comment in a document, absolutely! So shouldn't you be running each script file under that to retain the copyright information?

@fat
Copy link
Contributor

fat commented Jun 26, 2011

Actually uglify strips all comments -- we're manually adding that comment header back in.

Ender client code is included in each build.

If we ran each file through uglify independently there exists the potential for code conflicts -- imagine this scenario:

file 1 is minified and variables get renamed to var a, b, c.

file 2 is minified -- these variables too would get renamed to var a, b, c (because uglify has no idea about the other code).

@fat
Copy link
Contributor

fat commented Jun 26, 2011

Also -- that header serves the purpose of maintaining state for the ender cli tool. The Build: ender build qwery bean is how ender knows what packages are already built into your library, and with which options you are building your library.

@tbranyen
Copy link
Contributor Author

@fat i'm not convinced that issue exists. if you have two variables named the exact same thing in a global scope are you sure uglify is smart enough to discern and create separate unique variables? It seems this issue would happen if you included two source files anyways. Most JavaScript is written encapsulated in a closure.

Edit: I see what you're saying about the duplicate variables. Investing to see what can be done about global variables that have been shortened.

I'm not disputing having a header at all. I think its a great way to keep track of whats in the file without having to guess. I need to do more digging for this ender client side code and ensure that the copyright is directly above it.

@tbranyen
Copy link
Contributor Author

Okay after testing on my own and then reading the documentation for uglify (mangle names in the toplevel scope too (by default we don’t do this)):

> require('uglify-js')("(function(test, this, out) { })({},{},{});")
'(function(a,b,c){})({},{},{})'
> require('uglify-js')("var test = {}; var this = {}; var out = {};")
'var test={},this={},out={}'

It would appear your scenario should not exist.

@fat
Copy link
Contributor

fat commented Jun 26, 2011

Oh nice! However i'm concerned compiling 3 files separately and then concatenating them might not be as size efficient as running the minifier over all three files as a single entity. I'm fixing some tests for ender right now -- but if i get some more time, that would be interesting to check. Thanks for your help :)

@tbranyen
Copy link
Contributor Author

Well I mean file size will be increased because of the comments being added already, so having a slight increase because of some variable shortening will be negligible.

@ded
Copy link
Member

ded commented Jun 26, 2011

re: "but to put that in a file that doesn't contain any ender code seems weird don't you agree?"

the client is indeed embedded in the builds, even with the --sans case we have ender code sparsed throughout it... that's how ender works

@fat
Copy link
Contributor

fat commented Jun 26, 2011

comments aren't parsed by the interpreter though.

@beatgammit
Copy link
Contributor

Why not just leave out the comments altogether from the minified version? People shouldn't be reading the minified output anyway...

Maybe a simple: // packaged by Ender.js; please refer to the non-minified version for licensing information or something like that. That could get rid of the issue altogether.

@tbranyen
Copy link
Contributor Author

@beatgammit, That is very much an opinion.

I'm not contriving this issue. It must be remedied, before used in any production environment. Respecting developers licensing and attribution is very important in the open source community.

@beatgammit
Copy link
Contributor

@tbranyen- I think @fat's fix is pretty good. Of course, copyright notices must be available, which the unminified version provides. This takes the responsibility off of ender's shoulders and put's in on developers' shoulders to make that file available upon request.

That being said, stripping comments makes a lot of sense, especially for reducing the resulting file-size. Making the licenses publicly available is something the developer should have to worry about. Just sticking comments in an unreadable file is not sufficient in my mind, especially if the modules are under a restrictive license, like the GPL or a proprietary license.

@jdalton
Copy link

jdalton commented Jun 29, 2011

One way around Uglify (and minifiers in general) may be to replace designated comments with tokens similar to use strict directives like "ender: preserved comment block"; before minifying and them swap them out after.

Something like...

  var comments = [],
      token = '"ender: preserved comment block"',
      reMultiComments = /\/\*![\s\S]*?\*\//g,
      reTokens = RegExp(token, 'g');

  // replace comments with tokens
  source = source.replace(reMultiComments, function(comment) {
    comments.push(comment);
    return ';' + token + ';';
  });

  // minify
  source = uglify(source);

  // replace tokens with comment blocks
  source = source.replace(reTokens, function() {
    return '\n' + comments.shift() + '\n';
  });

@fat
Copy link
Contributor

fat commented Jun 29, 2011

Yeah i considered something like that. Unfortunately i don't think ender is to the point where many lib authors are ready to add special ender specific directives. I am still thinking about this issue however. We may be switching to closure which has "@preserve license" and/or I still may end up running each script through uglify separately and trying to preserve the top comment.

@jdalton
Copy link

jdalton commented Jun 29, 2011

@fat those are just tokens done during the minification process. The devs will only have to use the

/*!
 * Source.js <http://example.com/source/>
 * Copyright 2011 John Doe <http://example.com/>
 * Available under MIT license <http://example.com/mit>
 */

The key being the /*!

@mathiasbynens
Copy link

As mentioned before, YUI Compressor doesn’t remove /*! */ comments, but uglifyjs indeed does. Here’s the bug ticket for that: mishoo/UglifyJS#85
Eventually uglifyjs was changed to preserve only the first comment in the file (assuming that will always be a license/header type of thing), which is better than nothing but still far from ideal.

That said, wouldn’t what @tbranyen suggested be a viable solution?

Is it because you are concatenating all the files and then running it through uglify-js instead of individually and concatenating the result?

@fat
Copy link
Contributor

fat commented Jun 29, 2011

Ohhhh -- sorry i totally missed that! That's a great idea :) I'll give it try tonight - thanks

@fat
Copy link
Contributor

fat commented Jun 29, 2011

I prefer @jdalton's technique here because it has the advantage of compressing the code all at once and gives uglify a better change to optimize things.

@fat
Copy link
Contributor

fat commented Jul 1, 2011

ok ok ok... just a quick update -- i got a chance to squeeze this into my giant code cleanup branch... you can see it here: https://github.com/ender-js/Ender/blob/style-guide/lib/ender.file.js#L30

this means this will likely make it into the next release, barring dustin or i don't come up with something better.

@ded
Copy link
Member

ded commented Jul 1, 2011

that's way better

@benatkin
Copy link

Right now it says:

Module's individual licenses still apply.

What if it said:

Individual module licenses still apply.
Remove .min from the URL to see them.

Then people could arguably still comply with MIT, BSD, Apache2, etc if they didn't delete the non-minified file from the directory.

@fat
Copy link
Contributor

fat commented Jul 28, 2011

I fixed this already actually in a branch -- i just need to merge it in. Tomorrow i'll run more tests and merge during node night atttt twitterrrr

@fat
Copy link
Contributor

fat commented Jul 29, 2011

This is live now -- thanks again to @jdalton

@fat fat closed this as completed Jul 29, 2011
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants