Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contextify executor #16

Merged

Conversation

ericallam
Copy link
Contributor

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

@rehanift
Copy link
Owner

rehanift commented May 1, 2012

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
Copy link
Contributor Author

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

@rehanift
Copy link
Owner

rehanift commented May 1, 2012

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
Copy link
Contributor Author

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

Eric Allam added 3 commits May 2, 2012 14:45
@ericallam
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

rehanift commented May 3, 2012

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
Copy link
Contributor Author

Thanks :)

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

@rehanift rehanift merged commit 8c7d559 into rehanift:feature/use-contextify May 3, 2012
@rehanift
Copy link
Owner

rehanift commented May 3, 2012

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants