Option to auto-call process.binding() when calling RegisterModule() #38
Conversation
src/node.cc
Outdated
| node_module_register(module); | ||
|
|
||
| if(auto_bind) { | ||
| Evaluate(name + " = process.binding('" + name + "')"); |
There was a problem hiding this comment.
Hmm. Should we check name for invalid characters?
There was a problem hiding this comment.
from what I can see, nodejs' Binding function (defined in node.cc, invoked by process.binding()) throws a JS exception when no module named name is found, so this check is made there already.
ATM, I don't really know how node's exceptions work, so if you don't have a solution to handling these at this moment, I'd say we attach it to issue #17 for now.
There was a problem hiding this comment.
or open a new issue for exception handling (vs. MaybeLocal stuff)
There was a problem hiding this comment.
I was thinking about the case when invalid js code is executed:
std::name name = "123-foo"; // or "while(true) {}; foo"
Evaluate(name + " = process.binding('" + name + "')");
// Evaluate("123-foo = process.binding('123-foo')");
// Error: 123-foo isn't allowed as variable name.Also, maybe add const in front of the variable name in JS.
There was a problem hiding this comment.
Tried it out, throws a SyntaxError from node.js. Which makes sense, since all errors produced by javascript code (even by faulty JS code) result in JS exceptions/errors.
So again, (IMO) this should still be handled with these exceptions just like with every other call to Evaluate().
const is a good call.
|
closes #26 |
If string is given, module is bound to the JS variable named by the target string. If no string is given, the user has to call process.biding() themselves.
|
|
||
| void RegisterModule(const std::string & name, const addon_context_register_func & callback, void *priv) { | ||
| node::node_module* module = new node::node_module(); | ||
| void RegisterModule(const std::string & name, const addon_context_register_func & callback, void *priv, const std::string & target) { |
There was a problem hiding this comment.
IMHO it would be better to swap the priv and target parameters to match the other RegisterModule function
There was a problem hiding this comment.
i mean, i see what you mean, but even then the parameters won't match, right? one takes a callback, the other takes the <string, callback> map...
There was a problem hiding this comment.
Yeah, true. Maybe it's indeed not needed to swap the parameters.
There was a problem hiding this comment.
I will leave it as is then, I think. Is everything else in this PR ok? If so, I think we're ready to merge.
module's name in JS is given into RegisterModule() as a parameter. If no name is given, process.binding() will have to be called manually.