Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Clean up #24

Closed
wants to merge 11 commits into from

2 participants

@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
Owner

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
Owner

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
Owner

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"?

@tjanczuk
Owner

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
Owner

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
Owner

Closing this in lieu of #30

@tjanczuk tjanczuk closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
56 .jshintrc
@@ -0,0 +1,56 @@
+{
+ "globals": {
+ "_": true,
+ "after": true,
+ "async": true,
+ "afterEach": true,
+ "afterEach": true,
+ "Backbone": true,
+ "before": true,
+ "beforeEach": true,
+ "describe": true,
+ "expect": true,
+ "iit": true,
+ "it": true,
+ "jasmine": true,
+ "moment": true,
+ "runs": true,
+ "sinon": true,
+ "spyOn": true,
+ "waits": true,
+ "waitsFor": true,
+ "xit": true,
+ "xdescribe": true
+ },
+
+ "options": {
+ "asi" : false,
+ "bitwise" : true,
+ "boss" : false,
+ "browser" : true,
+ "curly" : true,
+ "debug": false,
+ "devel": false,
+ "eqeqeq": true,
+ "evil": false,
+ "expr": true,
+ "forin": false,
+ "immed": true,
+ "jquery" : true,
+ "latedef" : false,
+ "laxbreak": false,
+ "multistr": true,
+ "newcap": true,
+ "noarg": true,
+ "node" : true,
+ "noempty": false,
+ "nonew": true,
+ "onevar": false,
+ "plusplus": false,
+ "regexp": false,
+ "strict": false,
+ "sub": false,
+ "trailing" : true,
+ "undef": true
+ }
+}
View
61 .runJsHint.js
@@ -0,0 +1,61 @@
+var fs = require('fs');
+var jshint = require('jshint').JSHINT;
+
+var foldersToLint = [
+ './lib',
+ './test'
+];
+
+var utils = {
+ errors: 0,
+
+ readFile: function (filename) {
+ return fs.readFileSync(filename, 'utf8');
+ },
+
+ lintFile: function (filename, config) {
+ var results = jshint(this.readFile(filename), config.options, config.globals);
+ if (!results) {
+ this.errors++;
+ this.renderResults(filename);
+ }
+ },
+
+ resolveFiles: function(folders) {
+ var files = [];
+ folders.forEach(function (dir) {
+ var filesInFolder = fs.readdirSync(dir);
+ var jsFiles = filesInFolder.filter(function (file) {
+ return file.indexOf('.js') > 0;
+ }).map(function (js) {
+ return dir + '/' + js;
+ });
+
+ files = files.concat(jsFiles);
+ });
+
+ return files;
+ },
+
+ renderResults: function (filename) {
+ jshint.errors.forEach(function (e) {
+ if (!e) {
+ return;
+ }
+ console.error(filename + ': ' + e.code + ' ' + e.raw + ' at line: ' + e.line + ' character: ' + e.character);
+ });
+ }
+};
+
+var config = JSON.parse(utils.readFile('.jshintrc'));
+var filesToLint = utils.resolveFiles(foldersToLint);
+
+filesToLint.forEach(function (filename) {
+ utils.lintFile(filename, config);
+});
+
+if (utils.errors > 0) {
+ return process.exit(1);
+}
+
+console.log('Success: no linting errors.');
View
2  README.md
@@ -519,7 +519,7 @@ cd test
test.bat x64 0.8.22
```
-Would run tests agsinst node.js 0.8.22 on x64 architecture.
+Would run tests against node.js 0.8.22 on x64 architecture.
## Contribution and derived work
View
8 lib/edge.js
@@ -46,7 +46,7 @@ function normalizeEdgeSpec(options) {
else if (typeof options === 'function') {
options = { csx: options };
}
- else if (typeof options != 'object') {
+ else if (typeof options !== 'object') {
throw new Error('Specify the file name of the CLR DLL or provide an options object.');
}
@@ -97,13 +97,13 @@ function normalizeEdgeSpec(options) {
if (options.assemblyFile) {
if (!options.typeName) {
- var match = options.assemblyFile.match(/([^\\\/]+)\.dll$/i);
- if (!match) {
+ var matched = options.assemblyFile.match(/([^\\\/]+)\.dll$/i);
+ if (!matched) {
throw new Error('Unable to determine the namespace name based on assembly file name. ' +
'Specify typeName parameter as a namespace qualified CLR type name of the application class.');
}
- options.typeName = match[1] + '.Startup';
+ options.typeName = matched[1] + '.Startup';
}
}
else if (!options.typeName) {
View
34 package.json
@@ -7,25 +7,41 @@
},
"version": "0.7.4-pre",
"description": "Edge.js: run .NET and node.js code in-process",
- "tags" : ["owin", "edge", "net", "clr", "c#", "managed", ".net"],
+ "tags": [
+ "owin",
+ "edge",
+ "net",
+ "clr",
+ "c#",
+ "managed",
+ ".net"
+ ],
"main": "./lib/edge.js",
- "engines": { "node": ">= 0.6" },
- "licenses": [ { "type": "Apache", "url": "http://www.apache.org/licenses/LICENSE-2.0" } ],
- "dependencies": {
+ "engines": {
+ "node": ">= 0.6"
},
+ "licenses": [
+ {
+ "type": "Apache",
+ "url": "http://www.apache.org/licenses/LICENSE-2.0"
+ }
+ ],
+ "dependencies": {},
"devDependencies": {
- "mocha": "1.8.1"
+ "mocha": "1.8.1",
+ "jshint": "1.1.0"
},
"homepage": "https://github.com/tjanczuk/edge",
"repository": {
"type": "git",
"url": "git@github.com:tjanczuk/edge.git"
},
- "bugs" : {
- "url" : "http://github.com/tjanczuk/edge/issues"
+ "bugs": {
+ "url": "http://github.com/tjanczuk/edge/issues"
},
"scripts": {
"install": "tools\\install.bat",
- "test": "test\\test.bat"
+ "test": "test\\test.bat",
+ "jshint": "node .runJsHint.js"
}
-}
+}
View
47 test/101_edge_func.js
@@ -1,5 +1,4 @@
-var edge = require('../lib/edge.js')
- , assert = require('assert');
+var edge = require('../lib/edge.js'), assert = require('assert');
var edgeTestDll = __dirname + '\\Edge.Tests.dll';
@@ -13,35 +12,35 @@ describe('edge.func', function () {
assert.throws(
edge.func,
/Specify the file name of the CLR DLL or provide an options object/
- );
+ );
});
it('fails with a wrong parameter', function () {
assert.throws(
function () { edge.func(12); },
/Specify the file name of the CLR DLL or provide an options object/
- );
+ );
});
it('fails with missing assemblyFile or csx', function () {
assert.throws(
function () { edge.func({}); },
/Provide DLL or CSX file name or C# literal as a string parmeter/
- );
+ );
});
it('fails with both assemblyFile or csx', function () {
assert.throws(
function () { edge.func({ assemblyFile: 'foo.dll', csx: 'async (input) => { return null; }'}); },
/Provide either an asseblyFile or csx property, but not both/
- );
+ );
});
it('fails with nonexisting assemblyFile', function () {
assert.throws(
function () { edge.func('idontexist.dll'); },
/System.IO.FileNotFoundException/
- );
+ );
});
it('succeeds with assemblyFile as string', function () {
@@ -55,28 +54,28 @@ describe('edge.func', function () {
});
it('succeeds with assemblyFile and type name', function () {
- var func = edge.func({
- assemblyFile: edgeTestDll,
- typeName: 'Edge.Tests.Startup'
+ var func = edge.func({
+ assemblyFile: edgeTestDll,
+ typeName: 'Edge.Tests.Startup'
});
assert.equal(typeof func, 'function');
});
it('fails with assemblyFile and nonexisting type name', function () {
assert.throws(
- function () {
- edge.func({
- assemblyFile: edgeTestDll,
- typeName: 'Edge.Tests.idontexist'
- });
+ function () {
+ edge.func({
+ assemblyFile: edgeTestDll,
+ typeName: 'Edge.Tests.idontexist'
+ });
},
/Could not load type 'Edge.Tests.idontexist'/
- );
+ );
});
it('succeeds with assemblyFile, type name, and method name', function () {
- var func = edge.func({
- assemblyFile: edgeTestDll,
+ var func = edge.func({
+ assemblyFile: edgeTestDll,
typeName: 'Edge.Tests.Startup',
methodName: 'Invoke'
});
@@ -85,15 +84,15 @@ describe('edge.func', function () {
it('fails with assemblyFile, type name and nonexisting method name', function () {
assert.throws(
- function () {
- edge.func({
- assemblyFile: edgeTestDll,
+ function () {
+ edge.func({
+ assemblyFile: edgeTestDll,
typeName: 'Edge.Tests.Startup',
- methodName: 'idontexist'
- });
+ methodName: 'idontexist'
+ });
},
/Unable to access the CLR method to wrap through reflection/
- );
+ );
});
});
View
7 test/102_node2net.js
@@ -1,5 +1,4 @@
-var edge = require('../lib/edge.js')
- , assert = require('assert');
+var edge = require('../lib/edge.js'), assert = require('assert');
var edgeTestDll = __dirname + '\\Edge.Tests.dll';
@@ -29,7 +28,7 @@ describe('async call from node.js to .net', function () {
g: [ 1, 'foo' ],
h: { a: 'foo', b: 12 },
i: function (payload, callback) { }
- }
+ };
func(payload, function (error, result) {
assert.ifError(error);
assert.equal(result, 'yes');
@@ -72,7 +71,7 @@ describe('async call from node.js to .net', function () {
assert.throws(
func,
/Test .NET exception/
- );
+ );
});
it('successfuly marshals .net exception thrown on CLR thread from .net to node.js', function (done) {
View
21 test/103_net2node.js
@@ -1,5 +1,4 @@
-var edge = require('../lib/edge.js')
- , assert = require('assert');
+var edge = require('../lib/edge.js'), assert = require('assert');
var edgeTestDll = __dirname + '\\Edge.Tests.dll';
@@ -13,7 +12,7 @@ describe('async call from .net to node.js', function () {
var payload = {
hello: function (payload, callback) {
callback(null, 'Node.js welcomes ' + payload);
- }
+ }
};
func(payload, function (error, result) {
assert.ifError(error);
@@ -44,16 +43,16 @@ describe('async call from .net to node.js', function () {
assert.ok(result.g[1] === 'foo');
assert.equal(typeof result.h, 'object');
assert.ok(result.h.a === 'foo');
- assert.ok(result.h.b === 12);
+ assert.ok(result.h.b === 12);
callback(null, 'yes');
- }
+ }
};
func(payload, function (error, result) {
assert.ifError(error);
assert.equal(result, 'yes');
done();
});
- });
+ });
it('successfuly marshals data from node.js to .net', function (done) {
var func = edge.func({
@@ -73,7 +72,7 @@ describe('async call from .net to node.js', function () {
h: { a: 'foo', b: 12 }
};
callback(null, payload);
- }
+ }
};
func(payload, function (error, result) {
assert.ifError(error);
@@ -90,7 +89,7 @@ describe('async call from .net to node.js', function () {
var payload = {
hello: function (result, callback) {
throw new Error('Sample Node.js exception');
- }
+ }
};
func(payload, function (error, result) {
assert.ifError(error);
@@ -98,7 +97,7 @@ describe('async call from .net to node.js', function () {
assert.ok(result.indexOf('Sample Node.js exception') > 0);
done();
});
- });
+ });
it('successfuly marshals v8 exception in callback', function (done) {
var func = edge.func({
@@ -110,7 +109,7 @@ describe('async call from .net to node.js', function () {
process.nextTick(function () {
callback(new Error('Sample Node.js exception'));
});
- }
+ }
};
func(payload, function (error, result) {
assert.ifError(error);
@@ -118,5 +117,5 @@ describe('async call from .net to node.js', function () {
assert.ok(result.indexOf('Sample Node.js exception') > 0);
done();
});
- });
+ });
});
View
101 test/104_csx.js
@@ -1,5 +1,4 @@
-var edge = require('../lib/edge.js')
- , assert = require('assert');
+var edge = require('../lib/edge.js'), assert = require('assert');
var edgeTestDll = __dirname + '\\Edge.Tests.dll';
@@ -13,7 +12,7 @@ describe('csx', function () {
done();
});
});
-
+
it('succeeds with csx file with lambda', function (done) {
var func = edge.func(__dirname + '\\hello_lambda.csx');
func("JavaScript", function (error, result) {
@@ -33,14 +32,14 @@ describe('csx', function () {
});
it('succeeds with literal class', function (done) {
- var func = edge.func('using System.Threading.Tasks; public class Startup { '
- +' public async Task<object> Invoke(object input) { return "Hello, " + input.ToString(); } }');
+ var func = edge.func('using System.Threading.Tasks; public class Startup { ' +
+ ' public async Task<object> Invoke(object input) { return "Hello, " + input.ToString(); } }');
func("JavaScript", function (error, result) {
assert.ifError(error);
assert.equal(result, 'Hello, JavaScript');
done();
});
- });
+ });
it('succeeds with csx file with class', function (done) {
var func = edge.func(__dirname + '\\hello_class.csx');
@@ -49,7 +48,7 @@ describe('csx', function () {
assert.equal(result, 'Hello, JavaScript');
done();
});
- });
+ });
it('succeeds with cs file with class', function (done) {
var func = edge.func(__dirname + '\\hello_class.cs');
@@ -58,7 +57,7 @@ describe('csx', function () {
assert.equal(result, 'Hello, JavaScript');
done();
});
- });
+ });
it('succeeds with class in function', function (done) {
var func = edge.func(function () {/*
@@ -66,18 +65,18 @@ describe('csx', function () {
public class Startup
{
- public async Task<object> Invoke(object input)
- {
- return "Hello, " + input.ToString();
- }
- }
+ public async Task<object> Invoke(object input)
+ {
+ return "Hello, " + input.ToString();
+ }
+ }
*/});
func("JavaScript", function (error, result) {
assert.ifError(error);
assert.equal(result, 'Hello, JavaScript');
done();
});
- });
+ });
it('succeeds with custom class and method name', function (done) {
var func = edge.func({
@@ -88,10 +87,10 @@ describe('csx', function () {
{
public class Bar
{
- public async Task<object> InvokeMe(object input)
- {
- return "Hello, " + input.ToString();
- }
+ public async Task<object> InvokeMe(object input)
+ {
+ return "Hello, " + input.ToString();
+ }
}
}
*/},
@@ -103,70 +102,70 @@ describe('csx', function () {
assert.equal(result, 'Hello, JavaScript');
done();
});
- });
+ });
it('fails with malformed literal lambda', function () {
assert.throws(
function () { edge.func('async_foo (input) => { return "Hello, " + input.ToString(); }'); },
/Invalid expression term '=>'/
- );
- });
+ );
+ });
it('fails with malformed class in function', function () {
assert.throws(
- function () {
+ function () {
edge.func(function () {/*
using System.Threading.Tasks;
public classes Startup
{
- public async Task<object> Invoke(object input)
- {
- return "Hello, " + input.ToString();
- }
- }
+ public async Task<object> Invoke(object input)
+ {
+ return "Hello, " + input.ToString();
+ }
+ }
*/});
- },
+ },
/Expected class, delegate, enum, interface, or/
- );
- });
+ );
+ });
it('fails when Invoke method is missing', function () {
assert.throws(
- function () {
+ function () {
edge.func(function () {/*
using System.Threading.Tasks;
public class Startup
{
- public async Task<object> Invoke_Foo(object input)
- {
- return "Hello, " + input.ToString();
- }
- }
+ public async Task<object> Invoke_Foo(object input)
+ {
+ return "Hello, " + input.ToString();
+ }
+ }
*/});
- },
+ },
/Unable to access the CLR method to wrap through reflection/
- );
- });
+ );
+ });
it('fails when Startup class is missing', function () {
assert.throws(
- function () {
+ function () {
edge.func(function () {/*
using System.Threading.Tasks;
public class Startup_Bar
{
- public async Task<object> Invoke(object input)
- {
- return "Hello, " + input.ToString();
- }
+ public async Task<object> Invoke(object input)
+ {
+ return "Hello, " + input.ToString();
+ }
}
*/});
- },
+ },
/Could not load type 'Startup' from assembly/
- );
+ );
});
it('succeeds with System.Data.dll reference', function (done) {
@@ -179,10 +178,10 @@ describe('csx', function () {
public class Startup
{
- public async Task<object> Invoke(object input)
- {
- return "Hello, " + input.ToString();
- }
+ public async Task<object> Invoke(object input)
+ {
+ return "Hello, " + input.ToString();
+ }
}
*/}
});
@@ -191,5 +190,5 @@ describe('csx', function () {
assert.equal(result, 'Hello, JavaScript');
done();
});
- });
+ });
});
View
15 test/105_node2net_sync.js
@@ -1,5 +1,4 @@
-var edge = require('../lib/edge.js')
- , assert = require('assert');
+var edge = require('../lib/edge.js'), assert = require('assert');
var edgeTestDll = __dirname + '\\Edge.Tests.dll';
@@ -24,8 +23,8 @@ describe('sync call from node.js to .net', function () {
assert.ifError(error);
assert.equal(result, '.NET welcomes Node.js');
done();
- });
- });
+ });
+ });
it('successfuly marshals data from node.js to .net', function () {
var func = edge.func({
@@ -42,7 +41,7 @@ describe('sync call from node.js to .net', function () {
g: [ 1, 'foo' ],
h: { a: 'foo', b: 12 },
i: function (payload, callback) { }
- }
+ };
var result = func(payload, true);
assert.equal(result, 'yes');
});
@@ -57,7 +56,7 @@ describe('sync call from node.js to .net', function () {
assert.throws(
function() { func(null, true); },
/Test .NET exception/
- );
+ );
});
it('fails if C# method does not complete synchronously', function () {
@@ -69,8 +68,8 @@ describe('sync call from node.js to .net', function () {
}
*/});
assert.throws(
- function() { func(null, true) },
+ function() { func(null, true); },
/The CLR function was declared as synchronous but it returned without completing the Task/
- );
+ );
});
});
View
20 test/201_patterns.js
@@ -1,12 +1,11 @@
-var edge = require('../lib/edge.js')
- , assert = require('assert');
+var edge = require('../lib/edge.js'), assert = require('assert');
describe('call patterns', function () {
// Regression test for https://github.com/tjanczuk/edge/issues/22
it('two async callouts each with async callin (issue #22)', function (done) {
var log = [];
-
+
var callin = function (input, callback) {
log.push(input);
callback();
@@ -18,16 +17,16 @@ describe('call patterns', function () {
using System.Threading.Tasks;
public class Startup {
- public async Task<object> Invoke(IDictionary<string, object> input) {
- Func<object, Task<object>> callin = (Func<object, Task<object>>)input["callin"];
- await callin(input["payload"]);
- return input["payload"];
- }
+ public async Task<object> Invoke(IDictionary<string, object> input) {
+ Func<object, Task<object>> callin = (Func<object, Task<object>>)input["callin"];
+ await callin(input["payload"]);
+ return input["payload"];
+ }
}
*/});
var tryFinish = function() {
- if (log.length == 4) {
+ if (log.length === 4) {
assert.deepEqual(log, [ 'callin1', 'callin2', 'return', 'return' ]);
done();
}
@@ -42,5 +41,4 @@ describe('call patterns', function () {
callout({ payload: 'callin1', callin: callin }, callback);
callout({ payload: 'callin2', callin: callin }, callback);
});
-});
-
+});
Something went wrong with that request. Please try again.