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

Compile to ECMASCRIPT5 when using closure compiler #27

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@muzuiget
Copy link
Contributor

commented Aug 3, 2015

fix this https://bugs.dojotoolkit.org/ticket/16601, a bug 3 years ago.

the ticket above mark as a duplicate of https://bugs.dojotoolkit.org/ticket/16196, but this not right, if we write the optimizeOptions as

optimizeOptions: {
    languangeIn: 'ECMASCRIPT5',
},

then closure will throw an error

Done (compile time:0.098s). OPTIMIZER FAILED: InternalError: Java class "com.google.javascript.jscomp.CompilerOptions" has no public instance field or method named "languangeIn".

because this how dojo-util create the closure option object with this

    var options = new jscomp.CompilerOptions();
    for(var k in optimizeOptions){
        options[k] = optimizeOptions[k];
    }

the closure docs http://javadoc.closure-compiler.googlecode.com/git/com/google/javascript/jscomp/CompilerOptions.html#setLanguageIn%28com.google.javascript.jscomp.CompilerOptions.LanguageMode%29 says it should be call setLanguageIn(), and pass a typed-object to it. There noway to do this in optimizeOptions.

We don't need to find a new way to pass the option, because the dojo have code not compatible with ES3 now (at lease at v1.10), it can not complie to ES3

dojo.js.uncompressed.js:2033: ERROR - Parse error. IE8 (and below) will parse trailing commas in array and object literals incorrectly. If you are targeting newer versions of JS, set the appropriate language_in option.
    'dojo/aspect',
    ^    

dojo.js.uncompressed.js:85075: ERROR - Parse error. missing name after . operator
                return this.transformer.in(value);
                                        ^

dojo.js.uncompressed.js:94967: ERROR - Parse error. invalid property id
                var widget=new BorderContainer({gutters:false, class:"singleDecorator"});
                                                               ^

3 error(s), 0 warning(s)
Done (compile time:0.938s)

So, ES5 is only option.

@dylans dylans added this to the 1.11 milestone Aug 3, 2015

@dylans dylans added the bug label Aug 3, 2015

@dylans

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

I think two things should be fixed here:

  1. The things that are invalid ES3 should be fixed for Dojo 1.x (as the errors listed above are not ES5 specific features, but usage of reserved words and a trailing comma)
  2. ES5 should be a compiler/config option
@dylans

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

@muzuiget do you have a CLA on file? Please see the guidelines at https://github.com/dojo/util/blob/master/CONTRIBUTING.md#contributor-license-agreement if you do not.

@gitgrimbo

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2015

@muzuiget - Are you saying that Dojo itself has code that the closure compiler fails to compile?

If so, in the following lines of your uncompressed dojo layer, what are the module(s) to which these lines are associated? Can we determine that they are Dojo modules or your own application's modules?

dojo.js.uncompressed.js:2033
dojo.js.uncompressed.js:85075
dojo.js.uncompressed.js:94967

I ask because I haven't experienced Dojo modules being responsible for syntax errors during my own builds.

@dylans

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

Yeah, from what I can tell, the first one is somewhere with a list of AMD dependencies, the second one is using the reserved word in, and the third one should put the word class in quotes in a constructor. From what I can tell from the limited info provided, the first and third are easily fixable (it's possible the first one is a regression in Dojo), and I have no idea where the second one is coming from.

@gitgrimbo

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2015

An issue with the bug is that languangeIn is a typo, and leads to the error message provided:

Done (compile time:0.098s). OPTIMIZER FAILED: InternalError: Java class "com.google.javascript.jscomp.CompilerOptions" has no public instance field or method named "languangeIn".

When this is fixed (to languageIn), the actual error will be:

exception: (compile time:0.093s). OPTIMIZER FAILED: InternalError: Cannot convert ECMASCRIPT5 to com.google.javascript.jscomp.CompilerOptions$LanguageMode

So the main issue is probably that the Java type for languageIn (CompilerOptions$LanguageMode) is not a straight forward translation from any JS type. There are probably lots of optimizeOptions for which the type translation is simple (boolean, number, string, etc), and we probably don't see any issues for these.

@muzuiget

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2015

Oh, the typo is mistake, I actually correct it and tried again, but I copy and paste the wrong output code when create an issue.

And the ES3-compatible code is not Dojo, it a thirdparty library https://github.com/stemey/dojo-generate-form, sorry.

How about make whitelist to hardcode translate the option value type? I have update my code.

PS: @dylans Do I need to fill the form http://dojofoundation.org/about/claForm here, even required street address, It just one line code guy, I don't care my PR merge or not, just hope fix this issue in official code base.

@dylans

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

@muzuiget you're right in that we don't need a CLA for just a one line patch, but given my notes above to make it a flag rather than a hardcoded change, I think the eventual patch will need to be a bit more involved, hence my suggestion of getting a CLA in place now. The purpose of asking for a street address is to prove that you're a real human, we don't use the information we collect via the CLA process for any other purpose.

@gitgrimbo

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

Changes made in this area will also have to be applied twice.

Once to optimizeRunner.js (used in a 'Node build' when spawning a JVM to run closure):
https://github.com/dojo/util/blob/1.10.4/build/optimizeRunner.js#L86

And once to closure.js (used in a 'Java build'):
https://github.com/dojo/util/blob/1.10.4/build/transforms/optimizer/closure.js#L31

These two pieces of code (that are responsible for orchestrating the closure compiler) are almost identical, and could (should?) probably be refactored into a single module to be shared.

@dylans

This comment has been minimized.

Copy link
Member

commented Nov 28, 2015

Closed via 72cb7fd and 9d92e70. This could definitely use some additional testing (it passes for me).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.