Repository navigation
License comment between return and inlined function #434
Description
Bug report
terser 4.1.3
Complete CLI command or minify() options used
terser index.js --comments --compress
terser input
(function(){
/**
* @license
* Copyright 2018 The Incremental DOM Authors. 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
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
function isDocumentRoot(node) {
return node;
}
var getRootNode = function(node){ return node }
function getActiveElement(node) {
var root = getRootNode(node);
return isDocumentRoot(root) ? root : null;
}
function getFocusedPath(node, root) {
var activeElement = getActiveElement(node);
if (!node.contains(activeElement)) {
return [];
}
return activeElement;
}
getFocusedPath(document, document);
}())
terser output or error
Here the comment is between return statement and the inlined isDocumentRoot function
!function(){var node,activeElement,getRootNode=function(node){return node};node=document,document,activeElement=function(node){var root=getRootNode(node);return
/**
* @license
* Copyright 2018 The Incremental DOM Authors. 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
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
function(node){return node}(root)?root:null}(node),node.contains(activeElement)}();
Expected result
for instance,
!function(){var node,activeElement,getRootNode=function(node){return node};node=document,document,activeElement=function(node){var root=getRootNode(node);
/**
* @license ...
*/
return function(node){return node}(root)?root:null}(node),node.contains(activeElement)}();
Initially I faced this problem when tried to include incremental dom library into my project and build it with webpack in production mode. In the example above I tried to reduce the number of statements as much as possible, so the code itself doesn't have much sense, but its unminified version works without error
Activity
fabiosantoscode commentedon Aug 21, 2019
When a function is moved, comments are moved along with it. That's what makes the most sense for regular comments. Wrt license comments they shouldn't be moved, as they should usually be placed above all code.
I suppose we can move @license and @lic comments at the top of the code file to a special place and keep them there. But you have to place them at the real top of the file, not within an IIFE. How does that sound?
Martii commentedon Aug 21, 2019
@fabiosantoscode
Not sure if this is applicable, or what you are proposing for sure, but with some licenses, such as GPL, one can embed MIT into the middle of the module and those would need to keep the comment license below any starting code. I also included a brief code reference with snippet to parse the UserScript metadata block e.g. (uncompressed and ideal best, recommended, practices):
... This is why we keep all comments period when using minification techniques and preferably near where they are declared. We also have to have a specific key for OUJS for Authors to denote that minification (terser usage) may break things (EDITED: initially put it in the wrong metadata block... whoops and fixered). There are better ways to do this than embedding but it is always possible. There are also alternate methodologies to getting the UserScript metadata block but this is a common one.
sonic-interlock commentedon Aug 22, 2019
Not really good, since I don't have control over that library :)
Also, when webpack or another bundler creates a bundle, each module with its comments is usually placed within a function. I guess, similar error might happen with many other libraries
fabiosantoscode commentedon Aug 22, 2019
I see the problem with moving comments now.
This is kind of a complicated matter, in terms of catering to everyone's use cases.
There's also the matter of inlining/destroying functions which are the logical embodiment of the embedded library. There should be a way of saying "when this comment is found, the variable or function is not to be moved".
Additionally this is probably off-topic, but if I implement this behaviour I'll add a magic
/*@__NOINLINE__*///*#__NOINLINE__*/comment which prevents inlining of a declaration. This is helpful for those folks that need to create a function that shan't be inlined for v8/spidermonkey optimization purposes (maybe they don't want the rest of their code to be deoptimized due to a single function). The React team asked for this a while ago, I don't remember if it was on this repository or on UglifyJS2.Martii commentedon Aug 22, 2019
That sounds interesting much like the linters have a disabling between point A and point B. If you do end up with this feature you may want to consider "terser" like prefixing it in case there ever is or will be (i.e. future proof it) a conflict with those packages. I just got finished asking ESLint this and there might be other "hidden gems" (keywords) that I don't know about yet. JSHint uses plural in their docs but it too can use singular common names like
/* global someGlobalId */. There are more linters too so prefixing usually seems to be what they do to prevent comment identifier collisions with a few exceptions.Indeed! 😸 Most .user.js writers are fairly "lazy" (that usually includes myself) at using comment header licensing blocks but we enforce the bare minimum
// @license SPDX; optionalURLformat for brevity. I also had to open an issue a while back with a .user.js build routine (may be webpack related... don't recall atm) that uses uglify-js to pass the keep comments option because it was stripping all project licenses, esp. one library that I contribute to, which was not desirable. We worked it out so the final .user.js included the license statements. This is probably where this issue meets up with my replies here.fabiosantoscode commentedon Sep 25, 2019
But what would happen if some MIT code were to be inlined into non-MIT code?
I feel these questions wouldn't need to be asked if minified JS wasn't human-readable.
Martii commentedon Sep 25, 2019
@fabiosantoscode
I showed you GPL primary with MIT embedded up there along with JSLint and ESHint comments that can't be moved otherwise they fail to lint (
global/globalsis also part of the linters and those aren't even prefixed which can also cause issues if moved). Derivative code, regardless of compression/transformation/transpilation/etc. i.e. forking with terser/uglifying/mangling/etc., always has to include the licensing of the subordinate and the primary otherwise a DMCA could be created. That's the last thing we want on OUJS and I think I can safely presume the same for GF, GH, OUCSS, and any other hosting site.If terser starts "mangling" comments (without an opt-out and/or opt-in with a cumulative switch/API property/possible proposed, prefixed, inline "ignore" comment), including but not limited to positioning, and what the licensing applies to it make our jobs more difficult to respond to a DMCA... therefor they shouldn't be moved at all. This is one of the reasons why I have recommended the spec (that I didn't create but we do partially enforce) for over a decade to use the
@licensekey, in tandem with the@copyrightkey, for UserScripts/UserLibrary metadata blocks instead with a URL to the license (makes the source Code smaller too which is a consideration for storage store limitations in the browsers) however some licenses can be quite picky, including MIT, if the license headers are in place.P.S. There are also other JavaScript preprocessors that utilize comments and if you move them they'll break building if done on the fly and terser is applied as a pre-op (which we apply it as a post-op currently i.e. last stage).
fabiosantoscode commentedon Sep 25, 2019
Sorry. I meant Terser inlining MIT code into non MIT code. This can happen if the two sources are concatenated. Just an aside.
I find the concept of using Terser and then expecting something to be human readable and/or in order is quite the anthitesis of a lot of assumptions this project makes. Inlining functions and classes, reducing variables, collapsing declarations into one place, are all operations that move code mercilessly.
I wish to find the best way to implement this without incurring too much maintenance burden. When I get to this I'll hopefully have some ideas on how to do it.
Martii commentedon Sep 25, 2019
@fabiosantoscode
This is usually what all the .user.js engine managers do currently with
@require. There's been some talk of having the .user.js engine adding terser, or the like, to the cores. We prefer to keep libraries separate from the top-level script so we can optionally apply.minto the.user.jsand/or.minto the.jslibrary route. i.e.sanitized_name.user.jsvssanitized_name.min.user.js.If you want a bigger organization example take a look at https://code.jquery.com/jquery-3.4.1.min.js they keep some sort of licensing comment intact for distribution. jQuery minified is definitely not human readable but the licensing comment is.
That's always a good thing.
The preprocessor I stumbled upon just recently is metascript and it's still in use today. Doesn't mean that this is the only one out there. It also has point A to point B delineation comment syntax. If terser were applied in a build process before without an opt-out it could be disastrous for building/publishing on AMO/ATN/Chrome Web Store/etc. I think a more unique prefixing of your "magic" comments would be recommended, if you go that route, just to avoid the unknown and encourage cooperation with other preprocessors that might be staged in any order.
fabiosantoscode commentedon Sep 27, 2019
If any comments should have magic, they're the
@lic,@license,@preservecomments and a few more. I'm not happy to push a terser-originated magic comment into the wild. Piggy-backing on existing standards is probably wiser here.There was another magic comment thing that indicated the URL of the unminified source, but I don't remember which one it was. I can look it up when coding this.