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

1.16.3 by default generates at least ECMAScript 2015, while it was ECMASCRIPT3 in 1.16.2 and earlier #81

Closed
jandppw opened this issue Jul 13, 2020 · 7 comments · Fixed by #83

Comments

@jandppw
Copy link

jandppw commented Jul 13, 2020

In 1.16.2 -> 1.16.3, the closure compiler was updated.

1.16.2

jand@Desjani:pictoperfect-buildenv>java -jar src/uiBuild/yymmdd-HHMM/lib/util/closureCompiler/compiler.jar --version
Closure Compiler (http://github.com/google/closure-compiler)
Version: v20160911
Built on: 2016-09-13 16:51

1.16.3

jand@Desjani:pictoperfect-buildenv>java -jar src/uiBuild/yymmdd-HHMM/lib/util/closureCompiler/compiler.jar --version
Closure Compiler (http://github.com/google/closure-compiler)
Version: v20200112
Built on: 2020-01-13 22:21

That's quite a jump, of over 3 years.

A new build, created with util [email protected], fails on legacy infrastructure.

The reason is that the closure compiler outputs a more modern version of JavaScript. As an example, 1.16.3 outputs shorthand property names:

c.definitions = {
          Math,
          window,
          global: window,
          module: k.selfResolving(function(a, b) {
  …

where 1.16.2 does not:

root.definitions = {
…
		Math: Math,
		window: window,
		global: window,
		module: expression.selfResolving(function(mid, lazy){

As far as I know, shorthand property names are an ECMAScript 2015 feature.

From the closure compiler release notes, I learn that a command line property --language_out was added in release

The release notes for v20170806 say

August 6, 2017 (v20170806)


ECMASCRIPT_2017 is now the default language mode.

The output language defaults to ES5 instead of to the input language.

The release notes for v20170806 say

August 5, 2018 (v20180805)

You can now specify STABLE for input and/or output language level to request the latest version of JavaScript that is fully supported by the compiler. Currently, this means ES_2017 for input and ES5 for output.

I can find no other references to changes in the output language level in the release notes, so I need to assume that it is
still (intended to be) ES5. So either this is wrong, or the dojo build system is actively requesting another output language level. I cannot find prove of this. Yet, experiments show that the new setup by default generates at least ECMASCRIPT_2015.

Workaround

The more recent closure compiler supports a --language_out command line property;

v20200112 --help says:

 --language_out VAL                     : Sets the language spec to which
                                          output should conform. Options:
                                          ECMASCRIPT3, ECMASCRIPT5, ECMASCRIPT5_
                                          STRICT, ECMASCRIPT_2015, ECMASCRIPT_20
                                          16, ECMASCRIPT_2017, ECMASCRIPT_2018,
                                          ECMASCRIPT_2019, STABLE

After much searching, I found out that we can add a optimizeOptions property to our build profile build.profile.js:

…
  mini: true,
  optimize: "closure",
  layerOptimize: "closure",
  optimizeOptions: {
    languageOut: "ECMASCRIPT3"
  },
  stripConsole: "normal",
…

that generates a working build.

languageOut: "ECMASCRIPT5" generated a build that does not work, so it seems that 1.16.2 -> 1.16.3 does a jump from ECMASCRIPT3 to ECMASCRIPT_2015 or later.

The use of optimizeOptions in the build profile is not documented. But, in our build process at least, it is copied into bc, which is included from util/build/buildControl.js, which is used in util/build/transforms/optimizer/sendJob.js which calls util/build/optimizeRunner.js, where it is used as command line argument in the runJava call.

Suggestion

This is quite a jump for a patch version 1.16.2 -> 1.16.3.

I suggest

  • releasing a 1.16.4 where
    • the --languageOut parameter passed to the closure compiler defaults to "ECMASCRIPT3" (by adding it in util/build/buildControlBase.js, which would then be used both by util/build/transforms/optimizer/sendJob.js and util/build/transforms/optimizer/closure.js); alternatively, we can try to use the closure compiler option --browser_featureset_year
    • the use of optimizeOptions.languageOut in the build profile is documented
  • removing the default --languageOut parameter passed to the closure compiler not earlier than 1.17
@dasa
Copy link
Contributor

dasa commented Jul 13, 2020

Another good default for language_out could be NO_TRANSPILE . This is used here: https://github.com/ampproject/rollup-plugin-closure-compiler/blob/main/src/options.ts#L136

@dasa
Copy link
Contributor

dasa commented Sep 10, 2020

Another example of setting language_out to NO_TRANSPILE: https://github.com/emscripten-core/emscripten/blob/master/tools/building.py#L1055

@msssk
Copy link
Contributor

msssk commented Sep 11, 2020

@dasa thanks for the suggestion, this sounds reasonable.

@jandppw I think the default option should only be applied if the build profile has configured Google Closure Compiler as the optimizer. This logic has been implemented in #83. In my testing optimizeOptions is untouched if Closure is not configured or if optimizeOptions.languageOut is already set. If Closure is configured and optimizeOptions.languageOut is not configured then it will be set to 'NO_TRANSPILE'. Can you let me know if the changes in #83 work for your case?

@jandppw
Copy link
Author

jandppw commented Sep 11, 2020

@msssk Thx for the fix. I reviewed, and it looks ok to me. I do suggest a tad of documentation somewhere.

In our case, given the ancient target, I am glad to keep an explicit ECMASCRIPT3 in there.

I would be glad to test, but I cannot guarantee I will find the time over the next few weeks. Work on this project is intermittent. If a new dojo 1 version is released with the patch when we return to the project, it will be used. I'll see what I can do in the mean time, but don't hold off on our account.

I'm curious: if a 1.17 would be released, what are the plans for this this default?

@msssk
Copy link
Contributor

msssk commented Sep 11, 2020

After more investigation and consideration I think given the importance of legacy support for Dojo we should probably strive to maintain the same behavior for builds. Prior to 86f0042 Dojo's build behavior was:

  • expect ES3 input, log errors for non-ES3 code
  • produce ES3 output

The way to maintain this behavior is to provide explicit default settings (which can of course be configured in the build profile to different values):

{
    optimizeOptions: {
        languageIn: 'ECMASCRIPT3',
        languageOut: 'ECMASCRIPT3'
}

Unfortunately we can't completely provide the same behavior because the behavior of the Google Closure Compiler has changed. Prior to the introduction of block scope in ES6 Closure accepted function declarations within blocks without complaint. Once Closure added support for ES6 and block scope it became impractical to allow block-scoped functions in pre-ES6 code (see google/closure-compiler#3189).

In order for Dojo builds to work with the newer Closure releases and language set to 'ECMASCRIPT3' several modules that have block-scoped functions need to be converted to function expressions.

Application code that previously built fine will start seeing Closure compiler errors for block-scoped function use. There are two options to fix it:

  • refactor application code to avoid block-scoped functions
  • explicitly override optimizeOptions.languageIn to 'ECMASCRIPT6' or higher
    • this has the side-effect that you will not see warnings or errors for non-ES3 code

@dasa
Copy link
Contributor

dasa commented Sep 12, 2020

Neither option sounds good. An alternative would be to revert the Closure upgrade, and add support for Terser as an alternative optimizer.

@msssk
Copy link
Contributor

msssk commented Sep 14, 2020

@dasa agreed, after thinking about and discussing this more our current inclination is to set the default build configuration as follows:

{
    optimizeOptions: {
        languageIn: 'ECMASCRIPT_2017',
        languageOut: 'ECMASCRIPT3'
    }
}

This allows existing Dojo builds that work to continue working. What is lost is the ability to receive errors from the build process about ES5+ code (if you are expecting all your source to be ES3). We will update Dojo's code to remove use of block-scoped functions so that developers can change the build configuration to languageIn: 'ECMASCRIPT3' and as long as their application code is ES3-compliant then the build will succeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants