Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

mojito jslint does not flag syntax errors / invalid Javascript #533

Closed
dmitris opened this Issue Sep 19, 2012 · 7 comments

Comments

Projects
None yet
5 participants
Contributor

dmitris commented Sep 19, 2012

mojito jslint does not find or does not flag syntax errors in Javascript files which may lead to people wasting a lot of time hunting for bugs that can be found by simple source check.

Here's the steps to reproduce (compares "mojito jslint" with the output of Closure compiler):

  1. git clone git://github.com/yahoo/mojito.git

  2. copy examples/developer-guide/configure_routing into a testdir:
    cp -R examples/developer-guide/configure_routing /tmp

  3. in the testdir, modify the file mojits/RoutingMojit/controller.server.js with the following diff, introducing syntax errors and making Javascript invalid:
    22c22
    < var name="";

    val name=""; 
    

    45,46c45,46
    < "methods": methods.replace(/, $/,"")
    < };

      "methods": methods.replace(/, $/,"");
    }
    
  4. in testdir, run:
    $ mojito jslint
    path.existsSync is now called fs.existsSync.
    0 errors found.

  5. in testdir, run:
    $ find . -name '*.js' | xargs java -jar ~/CLOSURE_JAR_LOCATION/closure-compiler.jar --js
    ./mojits/RoutingMojit/controller.server.js:22: ERROR - Parse error. missing ; before statement
    val name="";
    ^

./mojits/RoutingMojit/controller.server.js:45: ERROR - Parse error. missing } after property list
"methods": methods.replace(/, $/,"");
^

./mojits/RoutingMojit/controller.server.js:48: ERROR - Parse error. missing ) after argument list
}, '0.0.1', {requires: ['mojito-url-addon']});
^

./mojits/RoutingMojit/controller.server.js:48: ERROR - Parse error. syntax error
}, '0.0.1', {requires: ['mojito-url-addon']});
^

4 error(s), 0 warning(s)

  1. in the original cloned directory (unmodified code), run the same closure command - the result is a bulb of minified code and no errors.

Would be great to modify "mojito jslint" or introduce additional option to the command-line mojito like "mojito syntax" to perform syntax validation of all Javascript and JSON files and flag all syntax errors.

Contributor

FabianFrank commented Sep 19, 2012

You are running mojito jslint wrong. You are doing
$ mojito jslint
which runs jslint for the mojito framework, not your app. You should run
$ mojito jslint app
if you want to lint your app.

Contributor

drewfish commented Sep 19, 2012

See mojito help jslint for more details. (Every mojito command has a mojito help which explains the command.)

Contributor

mojit0 commented Sep 19, 2012

I'd be good with a default that works inversely to that. Seems to me the default should serve customer application needs first.

ss

On Sep 19, 2012, at 2:40 PM, Fabian Frank wrote:

You are running mojito jslint wrong. You are doing
$ mojito jslint
which runs jslint for the mojito framework, not your app. You should run
$ mojito jslint app
if you want to lint your app.


Reply to this email directly or view it on GitHubhttps://github.com/yahoo/mojito/issues/533#issuecomment-8706080.

Contributor

dmitris commented Sep 20, 2012

agreed that the command-line usage seems counter-intuitive or framework developer centered, not user centered. As a user, you are more likely to lint your application, the files under current directory, rather than the mojito framework itself. Imagine that "make test" would run tests on the Makefile system and dependencies instead of the current directory!

But even running "mojito jslint app ." does not bring much help finding syntax errors. jslint spits lots of output even on the unmodified code example - and real syntax errors get buried among notices about wrong spaces and column numbers. Compare the following two gists:

https://gist.github.com/3754876 - jslint output, 116 lines, 57 errors found (!)
https://gist.github.com/3754881 - closure compiler, output, 16 lines, 4 errors, points directly to the lines with syntax errors

jslint does not seem the right tool if you want to make sure your Javascript code is syntactically valid and quickly locate the errors.

Contributor

dmitris commented Sep 20, 2012

agreeing with @mojit0 - the current command-line usage is counterintuitive, and it is also inconsistent. You run "mojito start" to start the server using the resources in the current directory, even if you try to do (the invalid) "mojito start app ". "mojito build" and "mojito compile", if I understand correctly, work on the files under the current directory (the help descriptions do not mention "mojito build app"). But test and jslint options require to pass "app ." to work on the current directory. I think the defaults should be modified: "mojito test" and "mojito jslint" should have "app ." as the default, and there should be separate options to test/jslint the framework itself - for example, "mojito test framework" or "mojito test --self". I will make a separate issue for that.

Contributor

mojit0 commented Sep 20, 2012

@dmitris I agree with all your points. A reimplementation/cleanup of the command line is one of our goals. We've almost completed work to resolve much of the trouble and inconsistency with mojito start vs. npm start that should land in the not-to-distant future. The rest is undergoing some design discussion but it's on our plate.

Contributor

rwaldura commented Jan 2, 2013

This issue now superseded by #537 .

@rwaldura rwaldura closed this Jan 2, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment