Opened 6 years ago

Last modified 23 months ago

#17177 assigned defect

Build system fails silently for hidden trailing comma error.

Reported by: Frank Fortson Owned by: Rawld Gill
Priority: blocker Milestone: 1.14
Component: BuildSystem Version: 1.9.0
Keywords: Cc:
Blocked By: Blocking:

Description

Dojo 1.9 release, closure compiler. When building an application, the target .js file is not created minified when there is this error, found only when you redirect the build's error output to a file thusly:

...\build --profile some.profile.js 1>std.txt 2>err.txt

app.js.uncompressed.js:13194: 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.

label: "Clear"

app.js.uncompressed.js:13199: 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.

"position":"absolute",

2 error(s), 0 warning(s)

The two trailing comma "errors" are not reported as errors in the build report file, nor in the command window error/warnings summary.

All the other files are built, but the target file (app.js) has just two lines (the two leading forward slashes for each line are not displayed below):

>>built @ sourceMappingURL=app.js.map

The uncompressed version that is created is correct and has the proper content.

The build-report.txt file showed this summary:

Process finished normally

errors: 0 warnings: 53 build time: 151.695 seconds

When removing the two trailing commas in 1) a new Button({...}) property list and 2) a domStyle(...) array, the correct target file (app.js) was "built" with proper content.

Although this was marked an error, it bubbled up as nothing or possibly a warning, yet the target file was not created properly.

It's good to catch these type errors early on, so it seems they should remain errors (although maybe tolerated by most browsers), but should bubble up in the build-report.txt file as an error, identifying the error(s) in the build-report.txt file with the same level of detail. [Side note: seems like other syntax errors that are in the report file do not provide enough info to resolve the problem, so resorting to the 1>std.txt 2>err.txt is the only way to investigate and resolve the problem. Is that the intent? If so, perhaps it should be documented?]

Change History (11)

comment:1 Changed 6 years ago by boazbn

I just wasted a day of work because of this... This must be fixed!

comment:2 Changed 6 years ago by DH

Another "error which doesn't get counted" that can likewise cause virtually-blank JS files is with code like:

function(foo,bar,baz,foo){...}

Where foo is present twice. This will cause a message like:

example.js.uncompressed.js:1: ERROR - Parse error. Duplicate parameter name "foo". ],function(foo,bar,baz,foo){

Again, the process *appears* to finish successfully and wrongfully *claims* to have zero errors...

comment:3 in reply to:  description Changed 6 years ago by DH

Replying to frankf:

...\build --profile some.profile.js 1>std.txt 2>err.txt

Side note: For something slightly more convenient, you can copy and interleave STDERR text into STDOUT by going:

command >log.txt 2>&1

Note that the order matters, and the 2>&1 needs to be after you've told it which file to write to, and that in some multi-threaded cases the two streams might constantly interrupt each other and lead to a mishmash of output.

comment:4 Changed 6 years ago by dylan

Milestone: tbd1.10
Owner: set to Rawld Gill
Priority: undecidedhigh
Status: newassigned

comment:5 Changed 6 years ago by DH

There might be a nodejs vs rhino thing going on here. My project is based on the third-party dojo-boilerplate template (which IIRC is recommended by some dojo tutorials) and the problem occurs when I do not have nodejs installed so it falls back on rhino (java.)

The following is an excerpt from dojo-boilerplate's build.sh script, just for context:

# With this code-branch, This correctly notices that those kinds of errors occurred node ../../dojo/dojo.js load=build --require "$LOADERCONF" --profile "$PROFILE" --releaseDir "$DISTDIR" $@

# This alternate code-branch didn't notice "comma errors" java -Xms256m -Xmx256m -cp ../shrinksafe/js.jar:../closureCompiler/compiler.jar:../shrinksafe/shrinksafe.jar org.mozilla.javascript.tools.shell.Main ../../dojo/dojo.js baseUrl=../../dojo load=build --require "$LOADERCONF" --profile "$PROFILE" --releaseDir "$DISTDIR" $@

After installing nodejs, I now see errors: 1 in the build-report.txt output when I have one-or-more ERROR - Parse error events.

comment:6 Changed 5 years ago by dylan

Milestone: 1.101.11

While this issue is very annoying, it does appear to be a Rhino only issue and not an issue with Node. As such, we're going to move to the 1.11 release as we don't yet have a patch to fix this for non-Node users.

comment:7 Changed 5 years ago by mc007

hi, for java based builds i could work out a patch in the closure compiler:

Dojo isn't really listening to the Rhino-Error reporter in that case, it actually just fills build-report.txt

  1. download gcc(latest) sources, and patch Parser.java with this replacement:

void maybeReportTrailingComma(Token commaToken) {

if (commaToken != null /*&& config.warnTrailingCommas*/) {

In ES3 mode warn about trailing commas which aren't accepted by older browsers (such as IE8). errorReporter.reportError(commaToken.location.start,

"Trailing comma is not legal in an ECMA-262 object initializer");

throw new RuntimeException?("Trailing comma is not legal in an ECMA-262 object initializer " + commaToken.location.start);

}

}

  1. compile gcc with ant jar
  1. replace Dojos's compiler.jar in utils/closureCompiler/compiler.jar with the version

that's it, now you get this as error in build-report.txt, looking like this:

error(356) The optimizer threw an exception; the module probably contains syntax errors.

module identifier: dijit/layout/ContentPane; exception: JavaException?: java.lang.RuntimeException?: Trailing comma is not legal in an ECMA-262 object initializer ContentPane?.js.uncompressed.js(18, 13)

alternative: just grab this patched version from here:

https://github.com/mc007/xjs/tree/dgrid_update/src/lib/util/closureCompiler

would be great to see that in official releases as this issues wasted many hours already.

ps: i tested this gcc patch against 1.6 and it seems all fine.

comment:8 Changed 4 years ago by dylan

Priority: highlow

comment:9 Changed 4 years ago by dylan

Milestone: 1.111.12
Priority: lowblocker

Rawld has started working on this. Unfortunately this is going to need to wait until 1.12 as we're out of time and the patched version of Closure compiler proposed above is having other issues in our tests unfortunately.

comment:10 Changed 3 years ago by dylan

Milestone: 1.121.13

Ticket planning... move current 1.12 tickets out to 1.13 that likely won't get fixed in 1.12.

comment:11 Changed 23 months ago by dylan

Milestone: 1.131.14
Note: See TracTickets for help on using tickets.