search

rehanift / engine.js

A scriptable task engine
MIT License
23 stars 4 forks source link

Contextify executor #16

Closed ericallam closed 12 years ago

ericallam commented 12 years ago

Added pending specs for the contextify integration (expecting locals and globals back on 'eval' event) issue #15

rehanift commented 12 years ago

Thanks for the spending specs! I'll try my best to pull this in and integrate the specs over the next couple of days.

ericallam commented 12 years ago

If you'd like I could take a crack at it.

rehanift commented 12 years ago

Definitely feel free to take a stab at this if you have the time. You may want to pull down my feature branch use-contextify and use that as your base. The native node vm module (which is in the current 0.2.4 version) does not give us some of the introspection into the sandbox context that we need, so contextify is a dependency.

If you do dig into this, maybe push to your feature branch regularly so we don't duplicate efforts?

ericallam commented 12 years ago

Yup, if I get started on this I will push regularly.

ericallam commented 12 years ago

Status so far: Got all the contextify specs working, as well as the other end-to-end tests. Added the TaskResponse valueish object to use in the callback. Have "SandboxError"'s being passed as the first argument to the eval callback.

Next up: fixing the unit tests.

ericallam commented 12 years ago

I'm not sure what the plan is on getting back out the locals from the context. From reading the contextify docs, it seems only globals can be returned.

rehanift commented 12 years ago

Wow, great work so far!

I've also been wondering what the best way to return locals from the context would be. I then found myself asking the question: "Do we need to return locals?". If your use-case doesn't need them, I'd say we leave them out for now.

If your use-case does need them I think we can come up with something, but Im having a hard time justifying "why" at the moment.

ericallam commented 12 years ago

Thanks :)

I don't think I need them, at least I haven't yet run into a situation where I need them.

rehanift commented 12 years ago

ok, so I got all the unit tests and end-to-end tests passing. Right now, the best way to run the end-to-end tests with with make deploy-and-test. This will simulate an npm install engine.js and run the unit and end-to-end tests against it. Unfortunately there is something with the new contextify integration that is relying on a node_module resolution (I think I did it this weekend) and make end-to-end-tests isn't working right.

Also, I had a bit of an abstraction leak in my head as I was 90% of the way through this. I crossed the wires with "errors relating to task execution" and "errors relating to code execution". Some cleanup on my feature/use-contextify branch is definitely in order.

Thanks for the great specs and getting the ball rolling on this.

ericallam commented 12 years ago

Good stuff. I see what happened with the "errors relating to task execution" and "errors relating to code execution" confusion, I'll submit a new pull request in a bit to fix those up.