Clean up

[alexbeletsky]

In sake of better supportability of project I did few things.

1. Added grunt build system few tasks (jshint, mocha)
2. Now, if grunt is installed (npm install -g grunt-cli) you are able to "one command" build app.
3. Build currently contains jshiting and running mocha tests.
4. Fixed all found Jshint issues.

@tjanczuk, you probably using VS to edit js files, so there are many "tabs on end line", "trainling spaces" errors, I would suggest you turn option to show your tabs/space, since JS does not like it :) (or use some better editors as Sublime + JSLinter).

If you like it, I'll update README and also try to add 'grunt-node-gyp", so build will be as easy asgrunt` command.

[alexbeletsky]

PS. I've added running node-gyp as build process, now to build up, just

grunt build

If working just with JS,

grunt

[tjanczuk]

Alexander, thank you for the submission. It would probably be a while before I got to that level of cleanup and automation.

I am not familiar with grunt. What is the advantage of grunt over regular npm scripts declared in package.json? In general I would like to avoid adding dependencies either in runtime or developement environment unless there is a smoking gun reasons to do so.

I started doing some automation work by automating the build of edge.js against any version of node.js (dynamically downloaded from http://nodejs.org/dist): check out tools\build.bar. I also added a mechanism to run tests against any node.js version and flavor that was previously built on the machine: check out test\test.bat.

When I look at what your pull requests brings vs what I am still missing in daily development, there are two things:

• the jshint checks,
• ability to build and run tests against all supported node.js versions and targets with one command (i.e. 0.6.20, 0.8.22, 0.10.0, each in x64 and x86 flavor). I thought one additional small script on top of tools\build.bat and test\test.bat would do it, probably wired up to npm.

Thoughts?

[alexbeletsky]

@tjanczuk regarding grunt - to understand you a point, image grunt as msbuild in node.js world. It's just default build system, with a lot of useful plugins inside (mocha, testing, watchning). The good part is that a lot of developers are familiar with it, so npm install and grunt after, is like pressing Ctrl+B in VS for .NET projects :)

.bat files are bad, event edge.js is not yet ready to be run on *nix boxes.. but I would still avoid them.

You current .bat files could be automated are grunt tasks. Currenlty I've added the simple one, just to execute node-gyp, but it could be only beginning.

You already have jshint check with this pull request, custom builds could be added as well. I can help with that,

[tjanczuk]

Most native modules in node.js are built with node-gyp. Once edge.js has support for Mono (#3), I expect node-gyp will provide a cross-platform building experience. Given that, what is the value grunt brings to the table beyond what node-gyp and npm offer? Is this an alternative, or does it add value on top of node-gyp+npm?

npm already runs mocha when you run npm test.

I do like the idea of jslint, but that can also be easily done with npm, right?

[alexbeletsky]

Tomasz, to make things a bit easier I would suggest to merge the stuff into local branch and just try

$ grunt build

npm is not a build system, rather package manager with ability to run scripts. Of cause, it's possible to run mocha and linting with npm, grunt will be beneficial since it easy to create batch jobs here - like, build includes node-gyp, jshint, mocha tests. So, instead of run $ npm build, $ npm jslint, $ npm test you just run grunt.

grunt is not alternative to node-gyp or something, it's just convenient way of building JS projects.

If you try and don't like it, let me know.

[tjanczuk]

Let me play with this for a day. I like the value add, but I need to convince myself I am OK with adding the dependency to get it.

Does grunt have some first class support for cross-platform (in the sense of "do A in Windows and B on Linux"?

[alexbeletsky]

Yes, it's possible http://jaketrent.com/post/an-environment-specific-grunt-build/

[tjanczuk]

Alexander, I thought about this hard and long and I think would like to hold off taking this pull request at this time. Once the mono support is in there may be critical mass to add this. But for the time being I expect only a handful of people to ever need to build edge.js (it comes pre-compiled in Git and npm), and the current set of tools adequate.

The one aspect I would love to see is automated run of jshint. If you are so inclined perhaps you could add an npm jshint script and a devDependency on package.json that allows running jshint over the javascript in the project?

[alexbeletsky]

The issue with throwing out grunt is - you have to implement similar tasks on your own. The functionality of jshint is limited to linting, so any kind of utilities you should do by your self.

I've added simple linting script as well as new npm script, so now you can run npm run-script jshit and see the results.

[tjanczuk]

Thanks for doing this. A few remaining issues:

• it appears I can no longer merge this PR cleanly. Could you fetch the latest and merge?
• could you move the .runJsHint.js and jshint.rc to the tools directory?
• the only change done by jshint in the *.js files I don't quite like is how it lays out the require statements in one line. Is there a way to tell jshint not to do this?

[alexbeletsky]

@tjanczuk I'll find a time to do that and send next pull request soon.

jshintrc, could not be moved to tools, many editors (Web Storm, Sublime) is looking for that file in the root of project and configure internal linters based on those rules. It's not a problem to move .runJsHint.js.

Regarding the require statements: my experience in JS shows that multiple var assigments is very bad practice. That's of cause matter of taste and habits, but for me it makes code hard to read and understand.

Ben Alman's post shows exactly my point.

I'm not saying you should use that, but please consider that. I don't remember what exact tweak to disable it, but I can search for it.

[tjanczuk]

Closing this in lieu of #30