Google Closure Compiler require issues
Reported by Benoît Ponsero | November 19th, 2011 @ 07:05 AM
all.js other.js other2.js /folder file.js
2 bugs detected:
-> 1: not work when call js in folder
in all.js :
will output the same line;
-> 2: require order inversed
will output "other2.js" before "other.js"
Comments and changes to this ticket
line 44 :
lazy val dependencies: List[File] = flatDependencies.reverse.distinct
// why reverse ?
line 60 :
val requireRe = """require("'["'])""".r
// perhaps just add "/" in regex like this : val requireRe = """require("'["'])""".r
I am not a scala expert, so i just give you some ideas.
I'd like to add the following description:
- Get the latest Play 2.0 src from the Github repo @ https://github.com/playframework/Play20
- Install according to Building from sources @ https://github.com/playframework/Play20/wiki/Installing
- Clone the test case from the Github repo @
- Start the play application inside the
googleClosureCompilerFailureTestCase directory from the command
- The failing test case is @ http://localhost:9000/
- The statically defined example of the expected result is @ http://localhost:9000/indexworking
Statement of Need
For large ecmascript applications, modular application definitions are a necessity. Modern web applications require more asynchronous interactivity which usually involves several large ecmascript applications. The Google Closure compiler allows for further optimizations by managing dependencies for the ecmascript author, among other optimizations.
- Get the latest Play 2.0 src from the Github repo @ https://github.com/playframework/Play20
Attached is a patch that fixes ticket-45.
I have verified against the test case I posted in comment 45-5.
Fixed reverse ordering issue by applying distinct to the flattened dependency val, then taking the tail of the resulting list, reversing it, and prepending it to the head of the flattend dependency list. This results in the correct order seen in the workingIndex example of the test in the comment 45-5 of the bug above.
Removed the incorrect leftover require('something') statements in the compiled js source by applying the source tree require regex's replaceAllIn to both the full source and the minified source.
Adding emacs autosave and ensime generated files to .gitignore (for those of us who use such things);-)
Fix Github repo location
The example fix is available for pull on Github if you would like me to issue a pull request to the play20 github project. Its location is : git://github.com/jackcviers/Play20.git
The fix branch is Play20Ticket45.
The github repo page is: https://github.com/jackcviers/Play20
Running against the test-case from 45-5
- clone the github repo above, or apply the attached patch to a
branch of the play20 repository. Your repo home will be referred to
as <PLAY20> from now on. The directory containing your
<PLAY20> repo will be called <WORKSPACE>.
- Go to <PLAY20>
user@yourhost $ cd
- If you are cloning my fork, pull the origin Play20Ticket45
user@yourhost $ git pull origin Play20Ticket45
- Go to the framework directory
user@yourhost $ cd framework
- Build a clean build and publish it locally.
user@yourhost /framework $ ./build clean publish-local
- Go back up to your <WORKSPACE> directory
user@yourhost /framework $ cd
- Clone the test project from comment 45-5
user@yourhost $ git clone git://github.com/jackcviers/googleClosureCompilerFailureTestCase.git
- The test project will be referred to as <TEST> from now
on. Go to <TEST>
user@yourhost $ cd
- Run the play build from <PLAY20>
user@yourhost $ /play clean test run
- Open a browser to http://localhost:9000. You should see the same working result as you see at the http://localhost:9000/indexworking route. The indexworking (/indexworking) route uses a statically defined script. The index (/) route uses the closure-compiled script. If you inspect the source, you will see that the requires have been placed in the correct order and that the require statements have been replaced by no-ops (';').
Questions and comments welcome
Please feel free to contact me for comment @ email@example.com or through my github profile. I am watching this issue, so if you would like me to make a pull request, I'd be happy to. Thank you.
Note that the largest issue with the requires is that it's incompatible with the Closure Library. Using a proprietary format will make this impossible to use with any existing libraries. Also, this seems to completely ignore deps files. I mentioned these issues to Erwan and he may look at Closure Library compatibility. In the meantime I'll likely create my own Closure Compiler Play Module based off https://github.com/benmccann/closure-compiler-utils
The require statement and modules in general have been under discussion for inclusion in the JS standard and are part of the CommonJS initiative.
From the ES wiki proposals section:
Makes sense. I'm happy as long as the Closure Library works. You may also want to talk to Nathan Naze about the solution. He's a Closure Tech Lead at Google (https://plus.google.com/115133512899590093795/about) and is interested in making the Closure compiler and library compatible with a common standard, but I believe he might have been working towards @requires (http://code.google.com/p/jsdoc-toolkit/wiki/TagRequires). He'd be very happy to hear what others think.
Lighthouse seems to have munged my link. The @requires link is http://code.google.com/p/jsdoc-toolkit/wiki/TagRequires
I did some more investigation and it turns out that the Closure Compiler has a command line option --process_common_js_modules:
This takes any CommonJS input and preprocesses it, so that it can be appended to Closure source code and then optimized. More details here:
It's quite important to me that we support code in the style of the Closure Library in any Closure Compiler plugin. Otherwise we cannot take advantage of 80%+ of the Closure/CommonJS code that exists out there. Hopefully this --process_common_js_modules flag will be able to accomplish this goal.
Now that the Closure compiler has been upgraded to the latest version, 80% of the current logic can be removed and replaced with a couple of compiler options:
Letting the compiler handle all the require logic would be nice because we would share code with more projects and so end up with fewer bugs and also it would bring us closer to being able to use the Closure Library, depswriter output, and numerous other Closure compiler features which are all broken by the custom logic in the plugin.
That's pretty cool, I didn't know Closure Compiler had support for CommonJS modules. I'll have a look at it.
Do you know if Closure Compiler supports generating unminified source? That's important so developer can debug their code without having to work with a minified version.
Hmm, I think so, but I'm not quite sure the best way to do it. They're pretty responsive on the compiler mailing list (https://groups.google.com/forum/#!forum/closure-compiler-discuss). One thing that might work is source maps (https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_...)
However, I think there's a more fundamental change that you might want to make to the way the Closure compiler is set up. I haven't actually used it yet, so I'm just guessing by reading the source, but it looks like you are creating a compiled file for every input file? If so, this really isn't going to work very well for long. For one thing, the compiler already takes long enough on large projects. I've worked on projects with > 100 JS source files. If you were to invoke the compiler 100 times the project would slow to a crawl. Incremental compilation would help, but you still wouldn't ever want to do this because a clean build would be prohibitively slow. Whatever the compilation time is, you are essentially squaring it, so a large project that would take 1s/file * 100 files, would take an hour and a half to compile instead of a minute and a half. Also, if you tell the compiler what the entry points into the program are, it can do a much better job optimizing it. If you use the setManageClosureDependencies that I mentioned then the entry points will be required. This will need to be user specified as there is no way for the compiler or Play to know what it should be. Also, there are other things that will need to be user configurable such as the error checking. Every project I've worked on, the team has always wanted a different set of errors to be checked by the Closure compiler - some as warnings and some as errors. The compiler has support for at least 25 different errors. Maybe the thing to do is let the user specify the compiler options. E.g. in your Build.scala you could specify:
compilerOptions.setManageClosureDependencies(ImmutableList.of("module$main")); compilerOptions.setProcessCommonJSModules(true); compilerOptions.setWarningLevel(DiagnosticGroups.ACCESS_CONTROLS, CheckLevel.ERROR); compilerOptions.setWarningLevel(DiagnosticGroups.AMBIGUOUS_FUNCTION_DECL, CheckLevel.WARNING);
Then in your Build.scala, you could specify completely different options as another user. You could let the user specify a list of options - one per output file they wanted generated. I've never worked on a project with more than one output file though. Even when I worked on Google Spreadsheets and Google Analytics, which both had many hundreds of input files being passed to the compiler, we just created one output file. The compiler does a very good job of minimizing the output and it can be cached by the user, so they downloaded the whole thing on the first page they viewed it eliminated the need for downloading any JS on later pages.
Actually I don't create one file per input file, I ignore files prepended by an underscore (like we do to compile less files). So if you put an underscore at the beginning of files that are only dependencies but not entry points, you'll be fine. This is done in PlayCommands.scala.
Regarding dependencies: What I am doing in the version currently in git is manually parse the files to resolve the dependencies, and feed only those files to Closure Compiler (and do that for each entry point file). Now on a local branch I tried to use the CommonJS support you linked, so I feed all JS files, I give the entry point and the compiler is supposed to keep only what's necessary. But then I need to know what the dependencies are for the incremental compilation (or I invalidate all JS files when one is modified, whether it's a dependency or not).
But now I have a problem: the CommonJS support is adding goog.require and goog.provide everywhere, but obviously they're not defined. Am I missing an option somewhere?
Regarding configurable error levels: yes, it's definitely something we're going to do.
Oh, I see. I'm not a big fan of having to name files with an underscore. If you want to use any existing library then you need to rename all the files and change all the imports, which is a bit of a pain. This was something we were hoping could be different for the Less compiler as well. (There's a discussion thread that a few of us were participating in where we thought it might be nice to pass the paths as input to the Less compiler https://github.com/playframework/Play20/pull/93)
Anyway, back to your question... The CommonJS support is a bit new and so if there's any rough edges the author malte.ubl@gmail has said he's interested in hearing feedback. The Closure Compiler thinks in terms of goog.require and goog.provides since that's what the Closure Library uses, so when you pass it CommonJS it alters the source to use those in an intermediate compilation step and then compiles the modified source. One thing that should work would be to simply include base.js as the first input (it's important that it's the first in the list) to the Closure compiler. It's a bit of lengthy file, but that's fine because the compiler will simply remove any references to unused code in the output, so it will basically be entirely excluded form the output anyway.
Yes, maybe we can propose an alternative to the underscore thing. In the meantime I'm trying to get the CommonJS stuff working.
I published my work in a branch: https://github.com/erwan/Play20/tree/fullclosure
So far even when I include base.js it gets removed entirely from the output. I suspect Closure Library trim the code before doing the renaming, and consequently remove it suspecting nothing is used?
Hey Erwan, you were very close! Just needed one more compiler flag. Also, I was was wrong about base.js helping. Not sure how to issue a pull request to anything but the main Play20 repo, but here's how I fixed it:
- State changed from confirmed to resolved
OK, I have it working with Closure Compiler managing the dependencies. I realized that there is no way Closure Compiler can know what are really the dependencies, because it works by feeding every single file to the compiler then let it trim the dead code.
I believe the dependencies problems are fixed so I'm closing this bug. Feel free to open others if problems persist.
Awesome! I love this change. Thanks Erwan! Now if you just let the user pass in their own options we should be able to use Closure Library with it as well and I will be a very happy camper =)
Also, there's no real minification going on. To actually minify you need to set CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
I'm going to expose all these parameters, I just need to figure out the most elegant way to do it with SBT.
Hi Erwan. Thanks for making things a bit more customizable :-) What do you think about allowing the user to actually pass in an instance of CompilerOptions instead of a list of Strings? Right now, I can set only a very tiny portion of the compiler options that I'd like to set. I'm guessing there's about 40 compiler options to turn on additional error checking and minification. I'd like to turn most of them on for my project, but I think it will get unwieldy to make a string option for each of them. Also, it could be a bit brittle. It's not uncommon for a flag or two to be added or removed with subsequent releases of the Closure compiler. Using the CompilerOptions directly we wouldn't need code changes to support new flags and it would be typesafe (and Play 2's typesafety is one of my favorite features ;-) Finally, if the default CompilerOptions had options.setCommonJSModulePathPrefix(source.getParent() + "/") then I could provide my own CompilerOptions which did not set this flag and thus have the option of using the Closure Library, which I don't think is yet compatible with that flag.
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »