Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#8549 closed defect (fixed)

[patch] [cla] stripConsole breaks with matched parenthesis

Reported by: dante Owned by: James Burke
Priority: high Milestone: 1.4
Component: BuildSystem Version: 1.2.3
Keywords: Cc: Richard Backhouse, Adam Peller
Blocked By: Blocking:

Description

When stripConsole is used in the Dojo build, if a closing parenthesis is found, the regexp fails.

eg:

(function(){

	var returnValue = function(){
		return "This debugging statement should be removed with stripConsole";
	};

	// works:
	console.log(returnValue());
	console.log("This won't be seen");

	// breaks:
	console.log("hello :) world!");
	console.log(" anything /) with a paren ");
	console.log(/^\)$/.test("hi"));
	
})();

outputs after build:

(function(){

	var returnValue = function(){
		return "This debugging statement should be removed with stripConsole";
	};

	// works:
	
	

	// breaks:
	 world!");
	 with a paren ");
	$/.test("hi"));
	
})();

seems like a more clever regexp is needed, but I don't speak that language very well.

Attachments (10)

stripConsole.diff (22.3 KB) - added by Nick Fenwick 11 years ago.
stripconsole.js (894 bytes) - added by Nick Fenwick 11 years ago.
basis of a test run from module.js
stripConsole_010609.diff (25.7 KB) - added by Nick Fenwick 11 years ago.
New svn diff, rooted at trunk/util
stripcomplex.js (1.1 KB) - added by Nick Fenwick 11 years ago.
Updated mon june 1 with tricksy js
stripConsole_020609.diff (26.6 KB) - added by Nick Fenwick 11 years ago.
Latest diff from svn, rooted at trunk/util
stripcomplex.2.js (1.3 KB) - added by Nick Fenwick 11 years ago.
Updated stripcomplex with code aimed at 'undefined' replacement.
stripConsole_050609.diff (28.7 KB) - added by Nick Fenwick 11 years ago.
Latest diff from svn, rooted at trunk/util
stripConsole_110609.diff (28.6 KB) - added by Nick Fenwick 11 years ago.
Fixed Messages.properties.
stripConsole_120609.diff (28.1 KB) - added by Nick Fenwick 11 years ago.
Latest diff from svn, rooted at trunk/util
stripConsole.diff.30_06_09 (27.3 KB) - added by Nick Fenwick 10 years ago.
Diff of latest trunk with my fixes applied. Diff is relative to trunk/util.

Download all attachments as: .zip

Change History (51)

comment:1 Changed 11 years ago by dante

noteworthy: if you move the comment stripping code to before the code obfuscation step (optimizeJs), an error is thrown during comment removal about invalid syntax. That would at least alert developers stripConsole changed their code in breaking ways (notwithstanding cases like var a = foo
console.warn("bee!"); b = c; )

though it makes the file size of the stripConsole pass significantly higher thus requiring more memory, this is a non-issue after testing with the patch on #8563

comment:2 Changed 11 years ago by dante

also relates to #8010

comment:3 Changed 11 years ago by dante

Milestone: tbdfuture

need to figure out the best way to attack this.

we could (at a cost) check the ast of the newly console-removed code to ensure it still checks out and revert to the orig fileContents if an exception is found? should this process be "safe" and always work (unexpectedly leaving failed console.*() matches) or fail violently and force the dev to reconsider their debugging techniques?

comment:4 Changed 11 years ago by James Burke

I was thinking since do not have a real ast generator is to maybe do a character by character walk with a mini state machine in the matching parens code to find the proper ending brace. Not sure how that performs. Also the matching brace code may be in use in the xdomain loader, would want to confirm that use.

comment:5 Changed 11 years ago by Nick Fenwick

I've modified shrinksafe's org/dojotoolkit/shrinksafe/Compressor.java so that as well as outputting shrunk tokens, it recognises the start of a 'console' call and discards tokens until a matching close parenthesis is found. This means it's safe for all of the following:

console.dir(callback);
console.log("This is in CallMe...");
var atring = "woo";
console.debug("In say_hello " + woo + (1+2) + say_hello);

.. and all the 'breaks' stuff in the above test.

The only thing missing from my impl at the moment is that it doesn't know how to leave _some_ console calls in. I see buildUtil.js knows to leave in warn()/error() calls.

It seems cleaner to do this work in shrinksafe on a parsed source tree rather than regex based mayhem in the build utils.

Who needs to review my code before attempting a commit? (I'll pursue a CLA)

comment:6 Changed 11 years ago by James Burke

neek: once you get the CLA sent in, just attach a patch to this ticket and we can get it reviewed. Thanks for your interest and time!

Changed 11 years ago by Nick Fenwick

Attachment: stripConsole.diff added

Changed 11 years ago by Nick Fenwick

Attachment: stripconsole.js added

basis of a test run from module.js

comment:7 Changed 11 years ago by Nick Fenwick

I fetched source with:

svn co http://svn.dojotoolkit.org/src/view/anon/all/trunk

Have modified shrinksafe to receive arguments as described in the build scripts, with new tests in shrinksafe/tests. Also modified build scripts to remove the internal stripConsole() function, and instead pass the stripConsole parameter into the call to shrinkSafe.

The .diff is from trunk/utils, and the two new .js files go into trunk/utils/shrinksafe/tests.

I hope it's all there :)

comment:8 Changed 11 years ago by Nick Fenwick

By the way, I think phiggins was going to review this. I've tested that a build still completes, but I'm not 100% sure files like dojo.js are properly stripped. When I put a test console.debug into dojo.js and run the build, my release dojo.js doesn't contain what I'd expect, though it looks like a good build.

I'll test some more with a local build but a going over by a guru would be a good idea.

comment:9 Changed 11 years ago by Nick Fenwick

I've done a profile=standard build and some console statements I put into dijit/form/Button.js are stripped out ok, so it seems to be performing as I intended.

		console.debug("This is a debug test.");
		console.error("This is a error test.");

A build with no stripConsole argument produces a release with the debug statement removed from dijit-all.js, but the error statement still in:

[neek@uberneek release]$ fndjs -c 'This is a error test' | grep '\:[0]$' ./testytest/dijit/form/Button.js:1 ./testytest/dijit/dijit-all.js.uncompressed.js:1 ./testytest/dijit/dijit-all.js:1 [neek@uberneek release]$ fndjs -c 'This is a debug test' | grep '\:[0]$' ./testytest/dijit/form/Button.js:1 ./testytest/dijit/dijit-all.js.uncompressed.js:1

A build with stripConsole=all results in a release with no console statements in dijit-all.js:

[neek@uberneek release]$ fndjs -c 'This is a error test' | grep '\:[0]$' ./testytest/dijit/form/Button.js:1 ./testytest/dijit/dijit-all.js.uncompressed.js:1 [neek@uberneek release]$ fndjs -c 'This is a debug test' | grep '\:[0]$' ./testytest/dijit/form/Button.js:1 ./testytest/dijit/dijit-all.js.uncompressed.js:1

Should the default for 'stripConsole' to build.sh be 'none' or 'normal'? i.e. would we prefer an absense of "stripConsole=" in the release.sh arguments to leave all console statements alone ('none'), or by default remove the debug calls ('normal')?

comment:10 Changed 11 years ago by James Burke

neek: Thanks for putting this together, even with unit tests! I will ask some people more familiar with the latest shrinksafe to also take a review of the code.

If no stripConsole is passed, then in the JS code for the build system, kwArgs.stripConsole will be an empty string, and that means "leave all console statements alone".

For the stripcomplex.js tests, it would be good to add things like this:

//These should pass, but just to be sure.
console.log("hello :) world!");
console.log(" anything /) with a paren ");
console.log(/^\)$/.test("hi"));

//It would be interesting to see how this comes out:
if(true)
    console.log("foo")
else
   var two = "two"

true ? console.log("bizarre") : (var bar = true);

comment:11 Changed 11 years ago by Adam Peller

I suggest something like "normal", "warn" and "all" for options. "none" seems redundant with not specifying the switch, and I don't see the use case for stripping out errors but not warnings.

comment:12 Changed 11 years ago by Adam Peller

Cc: Richard Backhouse added

comment:13 in reply to:  10 Changed 11 years ago by Nick Fenwick

Replying to jburke:

neek: Thanks for putting this together, even with unit tests! I will ask some people more familiar with the latest shrinksafe to also take a review of the code.

np, thanks for the thanks.

If no stripConsole is passed, then in the JS code for the build system, kwArgs.stripConsole will be an empty string, and that means "leave all console statements alone".

OK, and I agree with your other comment about the options. I had tried to mimic the original build script's arguments (hence normal,error) but they weren't very sane. Improved version coming up:

  • options are now [ "" | "none" ] | "normal" | "warn" | "all"
  • webbuild/server/js/* also fixed up, I missed them and they would break when used, though I've no idea what calls them. They are now hardcoded to use stripConsole="" i.e. leave console alone. Should this be "normal"?
  • minor tweaks, regex effenciency improvement

For the stripcomplex.js tests, it would be good to add things like this:

[cut]

true ? console.log("bizarre") : (var bar = true);

Holy edge case, Batman, you're tricksy. From what I can tell, the return value of a console.debug call is 'undefined', so I've fixed the code to collapse console.*() calls that are _not_ followed by a semicolon to 'undefined', so the above now collapses to:

true?undefined:(_8=true);

The chomping of semicolons and linefeed after console calls remains. Good catch. I've updated tests/stripcomples.js and added a suitable test.

Changed 11 years ago by Nick Fenwick

Attachment: stripConsole_010609.diff added

New svn diff, rooted at trunk/util

Changed 11 years ago by Nick Fenwick

Attachment: stripcomplex.js added

Updated mon june 1 with tricksy js

comment:14 Changed 11 years ago by Nick Fenwick

Incidentally, there's still a bug lurking in my implementation, because it presumes too much that the console.*() call is safe to remove entirely. As I mentioned, it replaces a console call that isn't followed by a semicolon with 'undefined', but can still fail. e.g.

	true ? console.log("bizarre") : (bar = true);
	true ? (bar = true) : console.log("bizarre");

shrinks to

true?undefined:(_9=true);
true?(_9=true):})();

The second line is clearly broken. Without a better approach, we end up outputting "undefined" in place of _every_ console.*() call, which is far from perfect.

I'm not sure how one should reliably test for a console call that is freestanding. I know there may be newline tokens, and semicolon tokens.. perhaps a pattern like:

- if "console" encountered
  - hunt back until non-NEWLINE is found.  if SEMI, then clearBefore = true.
  - process console.*() as normal
  - at end, if followed by SEMI, then clearAfter = true.
  - if clearBefore AND clearAfter, collapse to nothing
    - else, output "undefined" to satisfy surrounding logic

Unfortunately the logic in "hunt back" is problematic, because some tokens are followed by arbitrary length data, can anyone think of a good way to do it that doesn't involve maintaining a stack of previous tokens?

comment:15 Changed 11 years ago by Nick Fenwick

I realised that it's not "hunt back" logic if we only want to know what the last significant thing was, we just need to keep track of what the last significant thing we encountered was. So, a fixed version of Compressor now remembers if the token before the 'console' was a semicolon or not. This works in all cases I can think of offhand, including:

	true ? console.log("bizarre") : (bar = true);
	true ? (bar = true) : console.log("bizarre");
	
	(function() {
		return console.debug("Debug return statement.");
	})();
	(function() {
		return console.error("Error return statement.");
	})();

which now shrinks to (-stripConsole=normal):

true?undefined:(_9=true);
true?(_9=true):undefined;
(function(){
return undefined;
})();
(function(){
return console.error("Error return statement.");
})();

The point is that any console call that was done in a context that seemed to require its return value is now replaced with 'undefined', and all others are reduced to nothing (and trailing semicolons chomped).

The only case where this should now screw up is where side effects are enclosed in a stripped console call, e.g.

	for (i = 0 ; i < 10 ; console.debug("i is " + (i++)) ) {
		;
	}

which is stripped to

for(i=0;i<10;undefined){
}

I feel we can live with that. Anyone who puts meaningful logic in their debug statements deserves pain.

I realise I keep attaching diffs and then improving my code. What kind of cutoff are the people reading this waiting for? I'll stick another diff on and update the test js again.

Changed 11 years ago by Nick Fenwick

Attachment: stripConsole_020609.diff added

Latest diff from svn, rooted at trunk/util

Changed 11 years ago by Nick Fenwick

Attachment: stripcomplex.2.js added

Updated stripcomplex with code aimed at 'undefined' replacement.

comment:16 Changed 11 years ago by Nick Fenwick

Forgot to click 'replace attachment by same name' .. stripcomplex.2.js should replace the earlier stripcomplex.js.

comment:17 Changed 11 years ago by Adam Peller

will this work with semi-colon insertion?

I think the last diff still has "normal,error" etc. You should be able to remove "none" as an option also -- let's keep the options as few as possible. I'd prefer we not even allow -stripConsole with a blank, since that's counter-intuitive, and the shrinksafe tool is stand-alone.

comment:18 Changed 11 years ago by James Burke

I have not looked closely but just want to be sure that if someone passes in the stripConsole argument values today to build.js that we do something reasonable to translate them to the new values. This work can just happen in the JS, not have shrinsafe java code do it, but want to be sure weird things do not happen if someone upgrades dojo using a script that uses the args we support in Dojo 1.3.

comment:19 Changed 11 years ago by Nick Fenwick

peller, you're right, I left some old checking code in there, now removed. The buildUtil.js now automatically converts old style args like normal,warn into modern args like warn, and issues a warning via the logger to tell people to update their scripts.

I had some trouble blocking the use of "-stripConsole=" on the command line because I forgot the defaultValue was set to "" and it took me ages to realise this and differentiate between not providing a stripConsole, and providing an empty stripConsole :) The js build scripts now refuse to accept an empty string argument properly.

If no stripConsole argument is given, the js build scripts pass stripConsole to the java as the javascript null variable, which comes through in java as a real null String, unlike js undefined which comes through as a Java String of "undefined", oddly.

So all appears well again. For now.

peller, what do you mean about semi colon insertion?

Changed 11 years ago by Nick Fenwick

Attachment: stripConsole_050609.diff added

Latest diff from svn, rooted at trunk/util

comment:20 in reply to:  19 Changed 11 years ago by Adam Peller

Replying to neek: Messages.properties still has references to the old args, otherwise I'm fine as long as Shrinksafe uses the new args and the build scripts continue to support the old style.

peller, what do you mean about semi colon insertion?

Just wondering if this works if someone leaves out a semi-colon on the instruction prior to the log, like

var a=1 console.log("message goes here");

If it doesn't, we may have to live with this?

comment:21 Changed 11 years ago by Nick Fenwick

Sorry for the lack of update, been a busy week.

I don't follow the semi colon thing still. The js you quoted isn't valid, Rhino fails to parse it before shrinksafe gets near stripConsole logic.

[neek@uberneek shrinksafe]$ cat > arse.js
var a=1 console.log("message goes here"); 
[neek@uberneek shrinksafe]$ java -jar shrinksafe.jar arse.js
Exception in thread "main" org.mozilla.javascript.EvaluatorException: missing ; 
before statement

I've been using my new compressed source in my build recently and it has been performing fine, so I think it's reliable.

Messages.properties updated.. I think I'd propose the diff I'm about to attach as final.


Incidentally, on the subject of automatically concatenating strings that are split over multiple lines, I ran into the problem today and thought I'd record what I found as it was rather annoying to trace. The original source in dojo.js:

                        if("+" == oper){
                                retFunc = _nextSibling(filterFunc);
                        }else if("~" == oper){
                                retFunc = _nextSiblings(filterFunc);
                        }else if(">" == oper){
                                retFunc = _childElements(filterFunc);
                        }

Is shrunk using this regex:

replacement = source.replaceAll(
            		"\"([^\\\\\"]*(?:\\\\.[^\\\\\"]*)*)\"\\+\"([^\\\\\"]*(?:\\\\.[^\\\\\"]*)*)\"", str);

to the following:

if(==oper){_1c6=_1bb(_1c7);}else{if("~"==oper){_1c6=_1be(_1c // ... etc. etc. etc.

The initial "+" was squished. I'm not going to pursue this problem at this time. I still feel it would be best approached by hunting Tokens using the parsed source (string, whitespace and the + operator are all tokens), rather than this rather brutish regex attempt. I'd rather implement that than re-approach that horrible regex :)

Changed 11 years ago by Nick Fenwick

Attachment: stripConsole_110609.diff added

Fixed Messages.properties.

comment:22 Changed 11 years ago by Nick Fenwick

Hold on. Cometd seems to be broken in my release build. Don't know why yet.

comment:23 in reply to:  21 Changed 11 years ago by Adam Peller

Replying to neek:

I don't follow the semi colon thing still. The js you quoted isn't valid, Rhino fails to parse it before shrinksafe gets near stripConsole logic.

[neek@uberneek shrinksafe]$ cat > arse.js
var a=1 console.log("message goes here"); 
[neek@uberneek shrinksafe]$ java -jar shrinksafe.jar arse.js
Exception in thread "main" org.mozilla.javascript.EvaluatorException: missing ; 
before statement

Sorry, my bad. I didn't escape the code. There should be a new line between the '1' and the 'console'.

var a=1
console.log("message goes here");

comment:24 Changed 11 years ago by Nick Fenwick

I see. That code shrinks like so:

[neek@uberneek shrinksafe]$ cat > peller.js
var a=1
console.log("message goes here");
[neek@uberneek shrinksafe]$ java -jar shrinksafe.jar -stripConsole all peller.js 
var a=1;

So I think we're safe.

I've found and fixed a problem with my algorithm which broke the comet code. Thank God for the shrinksafe,keepLines build option, which allows me to sanely diff two release builds. This code in cometd/_base.js:

if(data.version < this.minimumVersion){
    if (console.log)
        console.log("cometd protocol version mismatch. We wanted", this.minimumVersion, "but got", data.version);
    successful=false;
    this._advice.reconnect="none";
}

broke, because my algorithm presumed the mention of "console.log" is the start of a function call. I've fix it to expect an open parenthesis too, however, isn't that code a little broken anyway? Could we change it to just call console.warn, instead of the if() test for console.log? i.e.

if(data.version < this.minimumVersion){
    console.warn("cometd protocol version mismatch. We wanted", this.minimumVersion, "but got", data.version);
    successful=false;
    this._advice.reconnect="none";
}

Just a suggestion. It now shrinks to:

if(data.version<this.minimumVersion){
if(console.log){
}
successful=false;
this._advice.reconnect="none";
}

Diff coming up. I'm finding today that no console.anything output is appearing in firebug, but to be honest this has happened on and off for a while, and happens with or without a release build, so it must be something to do with my platform. Functionally, my app is now fine,

Changed 11 years ago by Nick Fenwick

Attachment: stripConsole_120609.diff added

Latest diff from svn, rooted at trunk/util

comment:25 Changed 10 years ago by Nick Fenwick

Bump. This fix is finished.

Diff: stripConsole_120609.diff
Two new files go into util/shrinksafe/tests: stripconsole.js and stripcomplex.js (on this trac ticket as stripcomplex.2.js)

comment:26 Changed 10 years ago by bill

Summary: stripConsole breaks with matched parenthesis[patch] stripConsole breaks with matched parenthesis

BTW trac (or at least our configuration of it) doesn't send notifications when you attach something to a bug, so a bump is always necessary.

In any case still need a CLA.

comment:27 Changed 10 years ago by Nick Fenwick

I've submitted a CLA, receipt was confirmed by Aimee Evans, email of 28 May 2009, cc dmachi.

comment:28 Changed 10 years ago by bill

Summary: [patch] stripConsole breaks with matched parenthesis[patch] [cla] stripConsole breaks with matched parenthesis

comment:29 Changed 10 years ago by James Burke

neek: I applied the patch to trunk, two patch sections did not immediately apply, but they were just comment sections, easy to merge. However, after doing the patch, then running ./build.sh in the util/shrinksafe directory, I ran the tests/runner.sh file and got exceptions in the test. Only by modifying Compressor.java to do the following did I avoid the exceptions, but I still got test failures:

         //This used to be just the stripConsole == null check
         if (stripConsole == null || stripConsole.equals("none") || stripConsole.equals("")) {
        	 // may be null if unspecified on Main cmd line
        	 stripConsoleRegex = null;
         }

Any ideas on the issue?

comment:30 Changed 10 years ago by Nick Fenwick

jburke.. you're right, I checked out the latest svn trunk, applied the patch, and got the same errors. Someone else had been jumping up and down on the trunk in the meantime, meaning the way the existing js tests called shrinksafe failed given my new java code.

So, I've applied my patch to the latest trunk and resolved conflicts, fixed my test harness code to look like the new stuff that's appeared on trunk, fixed failures, and produced new patch diff from what that gave me.

Don't forget the two new tests .js files as well as the diff.. the runner.sh fails with an unhelpful message if the input files it expects aren't there, bit me a couple of times :)

Changed 10 years ago by Nick Fenwick

Attachment: stripConsole.diff.30_06_09 added

Diff of latest trunk with my fixes applied. Diff is relative to trunk/util.

comment:31 Changed 10 years ago by Adam Peller

the compress test is passing strip console in the position for encodeunicode, I think? Not sure if we need to find a way to better pass these args... doing something like kwArgs in Java is a pain.

comment:32 Changed 10 years ago by Nick Fenwick

peller, are you referring to shrinksafe/tests/module.js:12

	return new String(Packages.org.dojotoolkit.shrinksafe.Compressor.compressScript(source, 0, 1, stripConsole)).toString();

This is calling the overloaded java method compressScript, which exists in several forms:

    public static final String compressScript(String source, int indent, int lineno, String stripConsole) {
and...
    public static final String compressScript(String source, int indent, int lineno, boolean escapeUnicode, String stripConsole) {
and...
    public static final String compressScript(String source, int indent, int lineno, boolean escapeUnicode, String stripConsole, StringBuffer debugData) {

So in the four argument form we use from the module.js it would call the first one, passing the stripConsole string in the fourth argument position. I'm presuming the type system between js and java manages such overloaded invocation correctly; at least the number of arguments should make a mis-call impossible.

The argument for doing something like kwArgs in Java is too much to get into right now :) Frankly the way we're calling static Java methods from Javascript is horrible and I'd far rather refactor Compressor.java to a more sensible Java object instantiation, with init params passed in at that time, then a single simple call to a compressScript() method. My unfamiliarity with shrinksafe prevented me from wide ranging alterations before now.

I'd like to see these changes merged onto HEAD before trying such a large refactor, having already been bitten by other merges making my merge problematic.

I'm away for 4 days from tomorrow (Saturday). Long weekend, woo!

comment:33 Changed 10 years ago by Adam Peller

neek - sorry, I didn't notice the new function signature. my bad. yeah, Java can do all that. Still, I wonder if this arg list might get longer... maybe it's not something we need to deal with now.

comment:34 Changed 10 years ago by James Burke

neek: thanks for the updated patch. I probably will not get a chance to look at it for a week or so, I am in the process of moving. However, if another committer wants to grab it before then, that would be great too.

comment:35 Changed 10 years ago by James Burke

Cc: Adam Peller added

Finally taking a look at this patch, sorry for the long delay. I am settled now after moving.

One thing this patch does is change the behavior of the build system: The stripConsole now only works if the file is optimized via shrinksafe or comment removals. So technically stripConsole leaves console statements in if optimize or layerOptimize are not set to shrinksafe or remove comments.

I can see where this is problematic since I think just running it through rhino and decompiling the source loses the comments. Although it would be good to confirm that. If we can run the code through rhino and preserve comments but take out the console calls, that would be ideal so we could support the case where shrinksafe/comment removal is not desired but console stripping is.

neek, rbackhouse or peller, any thoughts?

comment:36 Changed 10 years ago by Nick Fenwick

jburke, I think I see what you're getting at. It is not possible to specify stripConsole (which will force use of shrinksafe) and comments=<none> i.e. leave comments in the source.

Some thoughts:

An aim of this patch was absolutely to tie stripConsole ability to the use of shrinksafe, so you can only use stripConsole by also invoking shrinksafe. i.e. remove the nasty old method for removing console (old stripConsole) which was based on regexes. The better way of doing it within shrinksafe, thereby using a more reliable method of operating on parsed code that bypasses issues with multiline and commented javascript.

'optimize' will cause either shrinksafe or comment removal to be used. My understanding is that "optimize=comments" will do a simple regex job (see buildUtil.js:removeComments) whereas "optimize=shrinksafe" will use shrinksafe on files.

Independant of that, 'stripConsole' controls the stripConsole argument that is passed to shrinksafe, if it is used.

I keep running round in circles, as I'm not very familiar with dojo builds, having only done a few builds of limited scope, and am not sure what files are supposed to be processed in what way. If I do a build with a recent svn trunk and specify no optimize or stripConsole setting with the 'fx' profile, dojox/fx.js is still put through ShrinkSafe?.

Could dojo team please make decisions on how to rearrange the build options?

comment:37 Changed 10 years ago by James Burke

neek, I sent email to the dojo-contributors list last night, saying I will likely check in this fix even though technically it is not the same behavior as previous versions. I think it will not really cause any code breakage, and I think it is unlikely someone would not want a shrinksafe/comments removal but want console stripping. So I am going to give that another day to see if anyone complains, but I think it will work out.

On the build options: layers have a separate build setting called layerOptimize, you can get info on it by typing ./build.sh or ./build.bat by itself. It is there because of legacy reasons. We have talked about in the future just combining layerOptimize and optimize into one setting, but we likely cannot do that until Dojo 2.0 for backward compatibility reasons.

optimize=comments is actually run through rhino, by doing a compileString/decompileScript combination, comments get removed. This was chosen since it was more reliable than the regexp work done in JS. See buildUtil.optimizeJs, the else if(optimizeType == "comments") section.

Hmm, so maybe we need a specific entry point into shrinksafe for removing comments, and that entry point can call the stuff to remove console calls. But it looks like that console removal is tied to the shrinksafe compressor. That is a bit problematic. I can see someone wanting comment removal with console removal. Any thoughts on that?

comment:38 Changed 10 years ago by Nick Fenwick

"I can see someone wanting comment removal with console removal."

There's no problem wanting comment removal.. as soon as the code goes through shrinksafe, or rhino as you said, comments are removed, because rhino only leaves in valid js statements. But, as I said, you will not be able to use any stripConsole behaviour yet leave comments in. Fortunately, there is no switch combination that specifies to do that.

e.g. the 'comments' mention here is redundant:

./build.sh optimize=comments stripConsole=normal

because comment removal is implicit in the stripConsole call to ShrinkSafe?.

The use of 'optimize=shrinksafe' here is redundant, because any use of 'stripConsole' implies use of shrinksafe.

./build.sh optimize=shrinksafe stripConsole=normal

It still makes sense to keep the 'optimize=comments' option, but only when shrinksafe has not been specified, because comments will be lost with any use of shrinksafe. e.g., this is sensible:

./build.sh optimize=comments

However, this is slightly redundant:

./build.sh optimize=comments stripConsole=normal

It might be confusing to find that even with no 'optimize' specification, ShrinkSafe? is still used, i.e.

./build.sh stripConsole=normal

implies use of 'optimize=shrinksafe'. I just did a build with:

./build.sh action=release stripConsole=normal profile=base releaseName=base_none_normal

and dojo/dojo.js is fully shrinksafe'd with no comments or linefeeds left in it.

But then, even this:

./build.sh action=release profile=base releaseName=base_none_none

puts dojo/dojo.js through shrinksafe, and I'm not sure what 'intended' behaviour is :) There are not queries about shrinksafe, but rather the build procedure.

comment:39 in reply to:  38 Changed 10 years ago by James Burke

Replying to neek:

There's no problem wanting comment removal.. as soon as the code goes through shrinksafe, or rhino as you said, comments are removed, because rhino only leaves in valid js statements. But, as I said, you will not be able to use any stripConsole behaviour yet leave comments in. Fortunately, there is no switch combination that specifies to do that.

Replying to neek:

"I can see someone wanting comment removal with console removal."

There's no problem wanting comment removal.. as soon as the code goes through shrinksafe, or rhino as you said, comments are removed, because rhino only leaves in valid js statements. But, as I said, you will not be able to use any stripConsole behaviour yet leave comments in. Fortunately, there is no switch combination that specifies to do that.

My question was more about how the existing code works, before the patch -- stripConsole and the optimization features area strictly separate features. And as such, it is possible to ask for comment removal only along with stripping console calls.

However, with this patch comment removal and console stripping are not compatible. It would have been nice to allow it, but then I am not sure how common it really is. But it is supported in the code (without the patch applied).

So I can understand how the command line args will be confusing now that the patch only allows console stripping if the code goes through shrinksafe. I will just put in some documentation and a warning message that stripConsole is only available if optimize or layerOptimize uses shrinksafe.

We'll put that in, and I'll send a note to the lists to see if anyone complains. If the backlash is high enough, then I'll just put back in the regexp-based JS comment stripping and only use that for the non-shrinksafe pathways.

It might be confusing to find that even with no 'optimize' specification, ShrinkSafe? is still used, i.e.

As I hinted before, the optimize argument only applies to non-layer code. layerOptimize controls the optimization for the build layers (like dojo.js) and it is defaulted to shrinksafe. It comes from the build system keeping an "uncompressed" version of the build layers when an optimization level is set via layerOptimize (and usually is since the default is to shrinksafe). This was useful if you wanted to debug code that used a layer. And normally, if you built a layer you did not care about the other files in the output so much since the main JS loading would be via layers.

But I think for a Dojo 2.0, when we can break some of the arguments/APIs, then we can look at consolidating on one optimize argument.

I am hoping to land the patch today, thank you for your work and your patience.

comment:40 Changed 10 years ago by James Burke

Resolution: fixed
Status: newclosed

(In [19538]) Fixes #8549, thanks to patch from neek, CLA on file. Better stripConsole handling by using shrinksafe. This means stripConsole only works now with an optimization setting that uses shrinksafe, but the console handling is much more robust. \!strict

comment:41 Changed 10 years ago by Adam Peller

Milestone: future1.4
Note: See TracTickets for help on using tickets.