dojo / util

Dojo 1 - build utilities. Please submit bugs to https://bugs.dojotoolkit.org/
https://dojotoolkit.org/
Other
60 stars 105 forks source link

Update Closure Compiler version and add optimizeOptions #44

Closed JSMike closed 8 years ago

JSMike commented 8 years ago

Update Closure Compiler to v20160713 Add compilationLevel option to optimizeOptions Add all compilation level set functions to optimizeOptions Add languageOut option to optimizeOptions Update languageIn and languageOut options to use fromString enum function

jquerybot commented 8 years ago

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

:memo: Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

JSMike commented 8 years ago

FYI, I just sent the corporate CLA to cla@dojotoolkit.org

JSMike commented 8 years ago

For reference, the options added can all be found here: https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/CompilationLevel.java

dylans commented 8 years ago

CLA verified!

JSMike commented 8 years ago

@dylans is there a specific area that I should be documenting the changes/range of options for optimizeOptions?

JSMike commented 8 years ago

Also to note, these changes allow for transpilation from ES6 to ES3 when using the closure compiler by setting the languageIn and languageOut in your optimizeOptions

dylans commented 8 years ago

@dylans is there a specific area that I should be documenting the changes/range of options for optimizeOptions?

@JSMike probably within https://github.com/dojo/docs/tree/master/build

Also to note, these changes allow for transpilation from ES6 to ES3 when using the closure compiler by setting the languageIn and languageOut in your optimizeOptions

That's excellent (and very much needed)!

JSMike commented 8 years ago

should i close/reopen the PR to have the bot verify the CLA?

dylans commented 8 years ago

should i close/reopen the PR to have the bot verify the CLA?

No, the bot is only smart enough to handle individual CLAs, not corporate CLAs, so you may want to also register yourself individually. I manually changed the CLA label after verifying.

JSMike commented 8 years ago

I've added a PR for documentation here: https://github.com/dojo/docs/pull/143

vansimke commented 8 years ago

When I ran a basic build with just dojo, dijit, and dojox (profile below), I get several thousand errors. Most of which seem to be 'Cannot call method "hasOwnProperty" of undefined'. Do we need to start adding some settings in order to make this work with this change?

var profile = (function() {
    return {
        basePath: ".",
        releaseDir: "./release",
        action: "release",
        optimize: 'closure',

        packages: [
            {
                name: "dojo",
                location: "dojo"
            },
            {
                name: 'dijit',
                location: 'dijit'
            },
            {
                name: 'dojox',
                location: 'dojox'
            }
        ]
    }
})();
vansimke commented 8 years ago

Okay, I was able to fix the errors by introducing an empty optimizeOptions object in the build profile. I think that optimizeRunner.js should be modified to protect against the object being left out.

JSMike commented 8 years ago

Ok, I'll fix that.

JSMike commented 8 years ago

@vansimke Thank you for realizing that, I hadn't tried running a dojo build without optimizeOptions since making changes. If optimizeOptions isn't set it will now default to an empty object. I also replaced .hasOwnProperty calls with the in operator.

vansimke commented 8 years ago

Everything seems to be working now. Thanks @JSMike!

vansimke commented 8 years ago

Hello @JSMike,

We're ready to land this, but we do have one more request: can you change this PR to pull against master? Unfortunately, we cannot accept this change for 1.11, but it will be included in the upcoming 1.12 release.

Thanks!

JSMike commented 8 years ago

@vansimke Sure thing, I'll close this and send over the PR shortly