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

Linked native module initialization function receives module name instead of module object #4756

Closed
kaero opened this issue Jan 19, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@kaero
Copy link
Contributor

commented Jan 19, 2016

Linked and built-in native modules initialization function receive v8::String as second argument instead of v8::Object instance of Module. It makes impossible to export something except precreated exports object.

Dynamic modules initialization:
https://github.com/nodejs/node/blob/v5.4.1/src/node.cc#L2193-L2268

void DLOpen(const FunctionCallbackInfo<Value>& args) {
  // ...
  Local<Object> module = args[0]->ToObject(env->isolate());  // Cast
  // ...
  Local<Object> exports = module->Get(exports_string)->ToObject(env->isolate());

  if (mp->nm_context_register_func != nullptr) {
    mp->nm_context_register_func(exports, module, env->context(), mp->nm_priv);
  } else if (mp->nm_register_func != nullptr) {
    mp->nm_register_func(exports, module, mp->nm_priv);
  } 
  // ...
}

Linked modules initalization:
https://github.com/nodejs/node/blob/v5.4.1/src/node.cc#L2400-L2428

static void LinkedBinding(const FunctionCallbackInfo<Value>& args) {
  // ...
  Local<String> module = args[0]->ToString(env->isolate());
  // ...
  Local<Object> exports = Object::New(env->isolate());
  if (mod->nm_context_register_func != nullptr) {
    mod->nm_context_register_func(exports,
                                  module,
                                  env->context(),
                                  mod->nm_priv);
  } else if (mod->nm_register_func != nullptr) {
    mod->nm_register_func(exports, module, mod->nm_priv);
  }
  // ...
  cache->Set(module, exports);
  args.GetReturnValue().Set(exports);
}

nodejs/help#44

\cc @indutny

@kaero

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2016

Btw, node.js doesn't provide any public interface to initialize linked modules. I think it's possible to add the initialization of linked modules to require by hacking Module._load() a bit.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Jan 19, 2016

I think we'd entertain patches for the bug in LinkedBinding() but anything that touches lib/module.js will most likely get roundly rejected.

@kaero

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2016

anything that touches lib/module.js will most likely get roundly rejected.

Holy cow detected ;)

@bnoordhuis Is it safe to continue to use process._linkedBinding? May it be renamed it to linkedBinding to became more "public"? Any documentation about building node.js with linked modules will be nice to have, now this feature looks like the sacred knowledge.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Jan 19, 2016

It's not really public API, the intended audience are embedders and people rolling their own node packages. As such, the underscore prefix seems appropriate to me; we won't intentionally break it but neither will we bend over backwards to maintain backwards compatibility.

@jasnell jasnell closed this in 71470a8 Feb 2, 2016

scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016

module: pass v8::Object to linked module initialization function
Fixes: nodejs#4756
PR-URL: nodejs#4771
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Rod Vagg <r@va.gg>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.