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

Risks of discarding legal comments by default #2745

Closed
zarqman opened this issue Dec 14, 2022 · 10 comments
Closed

Risks of discarding legal comments by default #2745

zarqman opened this issue Dec 14, 2022 · 10 comments

Comments

@zarqman
Copy link

zarqman commented Dec 14, 2022

I'm wondering about the decision in 0.16.0 to start discarding legal comments by default.

The licenses for a large number of npms and other projects only allow redistribution (ie: publication on the web) if the license is somehow included. A condition to this effect is found in the widely used MIT and Apache 2 licenses, among others.

With this change, apps that are bundling for web use/distribution have high odds of accidentally forfeiting their legal right to distribute the bundled code unless they catch this change and take action to reenable legal comments.

Packages that are bundling for their own distribution often include a separate file with their licensing. However, when projects duplicate licensing info as code comments, it's likely because they are particularly sensitive to the legal notices not being lost. As such, they also likely want such notices included in their bundle so that any downstream rebundling properly includes them.

Between these two audiences, it seems to me that most of them will indeed need to turn legal notices back on and thus would be well served by having them then reenabled by default.

I do understand the rationale of avoiding the confusion caused by the previous default value of 'eof'.

What about changing the default to 'inline' instead? That would both avoid the prior confusion while also helping users of esbuild meet their legal obligations to use code bundled from dependencies.

@tbjgolden
Copy link

I've been looking over this:

The defacto standard has been to not remove comments with an exclamation at the start: /*! ... */, //! ..., or to remove comments containing: @license.

  • terser, uglify, support this option behaviour by default
  • google closure compiler does not remove these comments
  • babel minify supports @license by default, but not !
  • esbuild has this option --legal-comments but has it disabled by default
  • swc has this option compress.format.comments: "some" but has it disabled by default

Only esbuild and swc do not try to preserve legal comments by default - perhaps it would make sense for both projects to enable it.

@evanw
Copy link
Owner

evanw commented Dec 19, 2022

I realize that other tools do this, but it's really not a good way to distribute the licenses of your dependencies to your users. For example, if you bundle the TypeScript compiler and you include these comments, your code will be marked with this comment:

/*! *****************************************************************************
Copyright (c) Microsoft Corporation. All rights reserved.
Licensed under the Apache License, Version 2.0 (the "License"); you may not use
this file except in compliance with the License. You may obtain a copy of the
License at http://www.apache.org/licenses/LICENSE-2.0

THIS CODE IS PROVIDED ON AN *AS IS* BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION ANY IMPLIED
WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR PURPOSE,
MERCHANTABLITY OR NON-INFRINGEMENT.

See the Apache Version 2.0 License for specific language governing permissions
and limitations under the License.
***************************************************************************** */

This behavior is confusing because it looks like Microsoft is claiming copyright over all of the code you wrote. IMO these kinds of comments are still confusing even if they are placed inline with the minified code. It still looks like Microsoft (or whoever) owns the copyright to your code. I changed the default for this setting because I didn't want esbuild to be at fault for automatically stamping your code with the wrong license, which I guess is another kind of "risk" to consider.

@tbjgolden
Copy link

Would moving them to the bottom of the file by default be somewhat of a happy medium?

@evanw
Copy link
Owner

evanw commented Dec 19, 2022

It sounds like you didn't read my comment? I'm saying that esbuild automatically including these comments in output files by default (regardless of where they are) is potentially confusing and misleading because these comments can appear to be assigning the copyright of your code to another entity. The fact that it happens automatically is problematic because people may not inspect the minified output and may not even be aware that this problem is happening.

@tbjgolden
Copy link

I did read the comment above, but (perhaps unfairly) read "didn't want esbuild to be at fault for automatically stamping your code with the wrong license" to mean "deliberately ignoring the precedent because I think it's silly".

I'm saying that not including these comments in output files by default is potentially leading users into unintended legal trouble when bundling their own code and others code together - after all, users of other minification tools could reasonably assume that legal comments would be preserved when using esbuild (I know I did).

Perhaps a non-technical solution might instead be to update the esbuild docs to makes it clear that "esbuild strips all comments including ones with attribution and licenses by default", or words to that effect?

@zarqman
Copy link
Author

zarqman commented Dec 19, 2022

Here's what I think is being said:

  • Including legal notices by default, because they sometimes don't identify the library name, can make it appear that the library author is claiming copyright over the entire bundled output. It's not reasonable to expect people to inspect the minified output code to double-check and correct this.
  • Excluding legal notices by default will often cause the app author distributing the bundle to engage in copyright infringement. It's not reasonable to expect people to inspect the minified output to double-check and correct this.

This creates somewhat of a conundrum, as both positions are defensible. The only point of agreement is that a well-chosen default matters, as we cannot expect users to, by and large, be involved.

What about if, in EOF, Linked, and External modes, a 1 line "introduction" comment was added? It might be as simple as:
/*! Bundled legal notices and comments follow: */
And then the various legal comments would follow. That lead-in line would provide some context that what follows is sourced from elsewhere and may not apply to the whole file. It could be expanded further, but I'm trying for something concise.

Additionally, when 2 or more notices are included, which I'd guess is rather common, the presence of multiple copyright notices should help clarify that any individual notice cannot apply to the whole file. But even if there's just one, the introductory comment should help.

Then, change the default to EOF so that people who don't check this setting are still covered properly.

Would the introductory comment line, along with a default of EOF, address everyone's concerns?

Additionally, for popular components whose output doesn't include the library name, Issues or PRs could be opened to add the name. (By anyone--not implying that burden falls on any participants here.)

@evanw
Copy link
Owner

evanw commented Dec 19, 2022

Yeah that makes sense. I can try to rework the legal comments feature to present a more readable version of the bundled comments. It would probably also be helpful to try to say what packages the comments are from, or something to that effect.

@evanw
Copy link
Owner

evanw commented Dec 19, 2022

Right now I'm thinking of going with something that looks like this at the end of the file:

/*! Bundled legal notices and comments:

json-graphql-server/lib/json-graphql-server.client.min.js:
  (*! For license information please see json-graphql-server.client.min.js.LICENSE.txt *)

decimal.js-light/decimal.js:
  (*! decimal.js-light v2.5.1 https://github.com/MikeMcl/decimal.js-light/LICENCE *)

classnames/index.js:
  (*!
    Copyright (c) 2018 Jed Watson.
    Licensed under the MIT License (MIT), see
    http://jedwatson.github.io/classnames
  *)

scheduler/cjs/scheduler.production.min.js:
  (** @license React v0.20.2
   * scheduler.production.min.js
   *
   * Copyright (c) Facebook, Inc. and its affiliates.
   *
   * This source code is licensed under the MIT license found in the
   * LICENSE file in the root directory of this source tree.
   *)

inflection/lib/inflection.js:
  (*!
   * inflection
   * Copyright(c) 2011 Ben Lin <ben@dreamerslab.com>
   * MIT Licensed
   *
   * @fileoverview
   * A port of inflection-js to node.js module.
   *)

react-dom/cjs/react-dom.production.min.js:
  (** @license React v17.0.2
   * react-dom.production.min.js
   *
   * Copyright (c) Facebook, Inc. and its affiliates.
   *
   * This source code is licensed under the MIT license found in the
   * LICENSE file in the root directory of this source tree.
   *)

object-assign/index.js:
  (*
  object-assign
  (c) Sindre Sorhus
  @license MIT
  *)

moment/moment.js:
  (*! moment.js *)
  (*! version : 2.29.4 *)
  (*! authors : Tim Wood, Iskren Chernev, Moment.js contributors *)
  (*! license : MIT *)
  (*! momentjs.com *)
*/

@tbjgolden
Copy link

Wow, it even plays nice with that unorthdox moment.js attribution comment style 🚀 nice work!

@evanw evanw closed this as completed in 47dd4de Dec 19, 2022
@zarqman
Copy link
Author

zarqman commented Dec 19, 2022

Nice work, @evanw! Appreciate the thoughtful approach of identifying each source.

afcapel added a commit to hotwired/turbo that referenced this issue Jul 20, 2023
So that build tools like esbuild or rollup-plugin-license can detect the
banner and keep the license in the minified output.

The banner is only a few bytes, so bundle size is not a concern.

Ref. evanw/esbuild#2745
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

No branches or pull requests

3 participants