Node.js compatibility

[danielepolencic]

Disclaimer: this is no small change.

The existing library is browser-friendly, but doesn't play nice in Node.js. This is due to the fact that the library assumes there's a global context (i.e. window).

This pull request refactors the codebase to use module loading using es6 imports and browserify. The result is an UMD friendly build that works in the browser (window.b3), with AMD modules and in Node.js.

As a consequence of the refactor the tests now run in Node.js using the npm test command. Each test runs in an isolate environment and has to specifically require the module to test.

The concatenated and minified files are created as part of the publish step of npm (no need to keep compiled files in the codebase).

Also, this PR should enable with little effort:

• integration of TravisCI. since the tests run in the node, plugging in a proper CI is a breeze
• removing the Class utility in favour of ES6 Classes. This will simplify the overall codebase making it more terse and readable

I understand it's a big change and you might not like it.

I just think this is a great library and wanted to help.

[elicwhite]

I think this change is extremely well needed. I considered doing this myself a while ago but ended up deciding to just hack it to make it work in node. This is the way libraries should be written and it makes behavior3js a lot more legitimized to have it in package form built with modules.

Well done @danielepolencic

@renatopp What do you think?

[chrisl8]

Exciting work! This would also clear up issue #4

[renatopp]

Indeed a great addition. I need some time to review this now, I'm finishing a freelance to adapt the behavior3 editor to a robotics company, so when I'm done with this I check it out.

[danielkcz]

Very nice! Just wanted to use B3 in webpack bundle but run into issue with b3 variable not being defined. This will definitely solve it.

Just small suggestion, in package.json change the main key to point to src folder. This field should be used when building the thing from source file, not for a created bundle. Such file should be in a browser field instead as per spec

[danielkcz]

Actually just noticed that bundle is not even created on npm install simply because all dependencies required for building are included in devDependencies. These are not installed and build task fails there. Everything used for building should be in dependencies. The devDependencies are mainly for modules used solely during development, eg. tests and stuff.

[danielkcz]

Hmm and on top of that after installing dependencies, build actually doesn't work. It fails with _minify task like this:

stream.js:74
      throw er; // Unhandled stream error in pipe.
      ^
Error
    at new JS_Parse_Error (eval at <anonymous> (D:\workspace\behavior3-manager\node_modules\behavior3js\node_modules\uglify-js\tools\node.js:22:1), <anonymous>:1526:18)
    at js_error (eval at <anonymous> (D:\workspace\behavior3-manager\node_modules\behavior3js\node_modules\uglify-js\tools\node.js:22:1), <anonymous>:1534:11)
    at croak (eval at <anonymous> (D:\workspace\behavior3-manager\node_modules\behavior3js\node_modules\uglify-js\tools\node.js:22:1), <anonymous>:2026:9)
    at token_error (eval at <anonymous> (D:\workspace\behavior3-manager\node_modules\behavior3js\node_modules\uglify-js\tools\node.js:22:1), <anonymous>:2034:9)
    at unexpected (eval at <anonymous> (D:\workspace\behavior3-manager\node_modules\behavior3js\node_modules\uglify-js\tools\node.js:22:1), <anonymous>:2040:9)
    at semicolon (eval at <anonymous> (D:\workspace\behavior3-manager\node_modules\behavior3js\node_modules\uglify-js\tools\node.js:22:1), <anonymous>:2060:56)
    at simple_statement (eval at <anonymous> (D:\workspace\behavior3-manager\node_modules\behavior3js\node_modules\uglify-js\tools\node.js:22:1), <anonymous>:2240:73)
    at eval (eval at <anonymous> (D:\workspace\behavior3-manager\node_modules\behavior3js\node_modules\uglify-js\tools\node.js:22:1), <anonymous>:2113:19)
    at eval (eval at <anonymous> (D:\workspace\behavior3-manager\node_modules\behavior3js\node_modules\uglify-js\tools\node.js:22:1), <anonymous>:2073:24)
    at eval (eval at <anonymous> (D:\workspace\behavior3-manager\node_modules\behavior3js\node_modules\uglify-js\tools\node.js:22:1), <anonymous>:2827:23)

Why would you even use that _minify task? It definitely wont work since you need to use Babel due to your changes. Simple concat is not enough. I tried to run _build task instead, that goes through and creates something, but it's not transpiled. I am not familiar with gulp and don't use browserify anymore, so I cannot help much with that, but this needs more tweaking apparently.

[danielepolencic]

This PR requires a little bit of work to be 100% ready. But I'm waiting for @renatopp to review it before doing the last bits.

As for the library, you can compile with guild _build. This will generate a self contained bundle (UMD) in libs/behavior3-0.2.0.min.js.

Hope this helps.

[danielkcz]

@danielepolencic No it wont build that, as I said above, it makes something, but it's not transpiled with Babel and thus broken in the browser.

I just gave you the feedback what's wrong, these are facts and waiting for @renatopp does not make them go away :)

Since this PR should be about NodeJS compatibility and thus everyone would expect it works flawlessly with NPM, it's certainly something that needs to be fixed.

[danielkcz]

Also you might have missed it, but NodeJS 6.x already implements like 90% of ES2015 features. For that reason there is a Babel preset for NodeJS. This essentially means that what I said above regarding devDependencies is not entirely true. For use in NodeJS there shouldn't be any Babel included in a package and let consumer to build that based on their NodeJS version. That also means adding .babelrc file to .npmignore.

[danielkcz]

Found another small issue regarding case sensitivity vs unix systems. All modules in Composite folder have import Composite from '../core/composite'; while actual file is Composite. Similar issue is in BehaviorTree requiring ./tick instead of ./Tick;

[renatopp]

Hey guys, I'm back on the development of B3. I'm going to finish some modifications to it and then update the libraries.

I must include the nesting-bt feature and some architectural changes on the next release, so I'm going to use this pull request as basis.

[chrisl8]

Sorry to be a pest, but is there any hope of this moving forward? If not, should we move to a fork maintained by @danielepolencic ?

Any thoughts from anyone still following this?

Thank you.

[danielepolencic]

If there's interest in getting this merged, I'm happy to rebase and fix the conflicts.

[renatopp]

@chrisl8 not pest at all. It has been some time since I worked on the b3 project, and I am engaged on some other projects right now, so I don't think I will update it any time soon. I would be more than happy to assist you guys if you want to continue the development.

[chrisl8]

Questions: @danielepolencic Is there a branch/commit on a fork that I can pull from to test this? @renatopp If the conflicts are resolved are you willing to merge this pull request or should we fork this? Also, would it be possible to point npm at this repo when this is done? (Fixing Issue 3)

[danielepolencic]

We could have someone to join the project as collaborator.

I'm not interested in maintaining a fork and I'd rather contribute to this repo.

[renatopp]

I agree with @danielepolencic, I think it is better to add someone here than working on a fork.

[renatopp]

Just added you, @danielepolencic, as a collaborator.

[danielepolencic]

Thanks @renatopp.

It has been a while since I worked on this PR. I'll have a look and rebase/update.

[danielepolencic]

@chrisl8 I fixed the code. Would you mind checking it out and offering some feedback?

Fork: https://github.com/danielepolencic/behavior3js

You can build the library with:

npm install
$(npm bin)/gulp build

The (generated) library is under /libs.

[chrisl8]

@danielepolencic It works perfectly with my existing code.

[danielepolencic]

@renatopp can you add me as a collaborator on npm too, please?

[renatopp]

@danielepolencic Done too, let me know if there is any problem.

[danielepolencic]

@renatopp I tried to add Travis to the org, but I don't have enough permissions. Would you mind granting permissions? This is so that we can run tests on PRs automatically.

[renatopp]

@danielepolencic how do I do that? By adding the service on the project settings? Should we continue this via email? (my email is renato.ppontes gmail.com)