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

Already on GitHub? Sign in to your account

Introduce NODE_MODULE_INIT() to pass full `module` object to addons #4634

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
9 participants
Owner

rvagg commented Jan 21, 2013

Proposal to allow native addons have access to the full module object so they can optionally overwrite exports, with a single function for example. Currently they are restricted to operating on an existing object.

Keeps NODE_MODULE() intact so doesn't affect existing addons, introduces NODE_MODULE_INIT() as an alternative and perhaps NODE_MODULE() could be deprecated and replaced with NODE_MODULE_INIT() in the docs with

NODE_SET_METHOD(target->Get(String::New("exports")), "foo", Foo);

(or a new, more convenient macro perhaps?).

Thoughts?

@rvagg rvagg referenced this pull request in Level/leveldown Jan 21, 2013

Closed

export single function, move iterator() to instance #5

Raynos commented Jan 21, 2013

👍

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Jan 21, 2013

src/node.h
@@ -202,7 +202,8 @@ struct node_module_struct {
int version;
void *dso_handle;
const char *filename;
- void (*register_func) (v8::Handle<v8::Object> target);
+ void (*register_func) (v8::Handle<v8::Object> exports);
+ void (*module_register_func) (v8::Handle<v8::Object> module);
@bnoordhuis

bnoordhuis Jan 21, 2013

Member

Bump NODE_MODULE_VERSION, you're changing the struct's size and layout.

@rvagg

rvagg Jan 23, 2013

Owner

fixed 051945b

@bnoordhuis bnoordhuis and 2 others commented on an outdated diff Jan 21, 2013

src/node.cc
@@ -1869,7 +1869,11 @@ static gid_t gid_by_name(Handle<Value> value) {
}
// Execute the C++ module
- mod->register_func(target);
+ if (mod->register_func != NULL) {
+ mod->register_func(target->Get(String::New("exports"))->ToObject());
@bnoordhuis

bnoordhuis Jan 21, 2013

Member

I'd probably split this up and add a sanity check:

Local<Value> exports = target->Get(String::New("exports"));
assert(exports.IsObject());
mod->register_func(exports.As<Object>());
@rvagg

rvagg Jan 23, 2013

Owner

@bnoordhuis is this necessary since the call comes in from module.js where exports is explicitly defined?

@bnoordhuis

bnoordhuis Jan 23, 2013

Member

Defense in depth. Stuff like this helps catching regressions.

@TooTallNate

TooTallNate Jan 23, 2013

IMO I think both APIs should be supported. I have lots of old native
modules that I have no desire to update to this new API.

In the docs, I'd rather both macros be explained so that the user can pick
the one that best suits their needs (and doesn't mention one being older
than the other).

On Wednesday, January 23, 2013, Ben Noordhuis wrote:

In src/node.cc:

@@ -1869,7 +1869,11 @@ static gid_t gid_by_name(Handle value) {
}

// Execute the C++ module

  • mod->register_func(target);
  • if (mod->register_func != NULL) {
  • mod->register_func(target->Get(String::New("exports"))->ToObject());

Defense in depth. Stuff like this helps catching regressions.


Reply to this email directly or view it on GitHubhttps://github.com/joyent/node/pull/4634/files#r2741400.

@rvagg

rvagg Jan 23, 2013

Owner

ok, probably needs a new name to better indicate the difference, any suggestions?

Member

bnoordhuis commented Jan 21, 2013

The PR itself mostly LGTM but I kind of question its usefulness. You can accomplish the same thing with a JS shim, right?

Owner

indutny commented Jan 21, 2013

I agree with @bnoordhuis, in the most of cases you'll need to wrap it up in js-land to do some checks and create rich api.

Owner

rvagg commented Jan 21, 2013

This comes out of discussion on LevelDOWN where we want to have an almost completely native LevelDB addon that we can extend with LevelUP. We'd like to export a single function; this is emerging as the dominant pattern for Node modules so it's really a bit of an oversight that this is not possible from .node modules. Of course we can (and will) deal with it for now by exporting a function from the exports in the .node and then assigning that function as the exports of the main .js but it's a bit of a smell that you have to do it like that.

Also, this isn't just about being able to overwrite module.exports, there's more on module that would be good to have access to in native modules. And, surely this is a matter of consistency? Why only give .node modules the exports object but give .js modules the whole lot?

Member

bnoordhuis commented Jan 21, 2013

@isaacs and @TooTallNate, you should probably chime in. Is this something people complain about?

-1, I don't think you should ever be exposing your native modules directly to users. Just make a js file which exposes them however you want and have that be what users (or you) require.

As long as it's backwards compatible, I don't have any reason against it.

On Monday, January 21, 2013, Roman Shtylman wrote:

-1, I don't think you should ever be exposing your native modules directly
to users. Just make a js file which exposes them however you want and have
that be what users (or you) require.


Reply to this email directly or view it on GitHubhttps://github.com/joyent/node/pull/4634#issuecomment-12507259.

Member

juliangruber commented Jan 21, 2013

+1 I don't see a reason against this

Owner

rvagg commented Jan 21, 2013

@shtylman this isn't about exposing them directly to users; you can't be sure where the .node will end up anyway (e.g. Debug).

What is the advantage of having this over just attaching to the exports object and letting the js layer do the rest? To me it just seems like more ways to do the same thing. You can already export just a function from js, so wrap your binding and export that.

isaacs commented Jan 22, 2013

If it's an advantage when you expose it to users, then it's an advantage when you're the user. "Export one thing" is generally better.

I'm +1 on this feature, whenever @bnoordhuis or @TooTallNate is happy with the code.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Jan 22, 2013

src/node.h
#define NODE_MODULE(modname, regfunc) \
extern "C" { \
NODE_MODULE_EXPORT node::node_module_struct modname ## _module = \
{ \
NODE_STANDARD_MODULE_STUFF, \
regfunc, \
+ NULL, \
+ NODE_STRINGIFY(modname) \
+ }; \
+ }
+
+// regfunc(module) form
+#define NODE_MODULE_INIT(modname, regfunc) \
@bnoordhuis

bnoordhuis Jan 22, 2013

Member

Is there a better name for this macro? NODE_MODULE_INIT seems a little confusing; it suggests that NODE_MODULE doesn't perform initialization.

Also, it should be documented in doc/api/addons.markdown.

@rvagg

rvagg Jan 23, 2013

Owner

@bnoordhuis the reason I chose that name is that I imagine this being the primary interface for native addons with NODE_MODULE being deprecated. I'll do some work on addons.markdown and you can let me know what you think about that.

Owner

rvagg commented Jan 23, 2013

Updated addons.markdown to use NODE_MODULE_INIT and also a note about NODE_MODULE being the "old" form. How does it sound?

Owner

rvagg commented Jan 23, 2013

I've added the sanity assert() and I'll tweak the docs some more if we're going to keep both NODE_MODULE and this new one around; but it probably needs a new name since NODE_MODULE_INIT doesn't really tell you much about how it's different from NODE_MODULE. Any suggestions for a name that may indicate that it passes module rather than just exports?

@Lewuathe Lewuathe and 1 other commented on an outdated diff Jan 24, 2013

doc/api/addons.markdown
@@ -93,6 +97,13 @@ the recently built `hello.node` module:
Please see patterns below for further information or
<https://github.com/arturadib/node-qt> for an example in production.
+### `NODE_MODULE_INIT` vs `NODE_MODULE`
+
+In versions of Node prior to v0.10 the macro `NODE_MODULE` was used in place of
+`NODE_MODULE_INIT`; this macro would pass the `exports` object directly to your
+initialization function rather than the full `module` object. `NODE_MODULE` is
+still available but is deprecated and may be removed from a future version of Node.
@Lewuathe

Lewuathe Jan 24, 2013

I'm sorry for interrupting discussions, but I doubt it is necessary to remove NODE_MODULE macro in the future.
Anyway, it should not be put on until removing is settled, I think.

@rvagg

rvagg Jan 24, 2013

Owner

np @Lewuathe, it's in the process of being changed as @TooTallNate has pointed out that he'd rather it not be marked as deprecated (the intention of the message was just to encourage people not to use it, not to say that it will be removed, just give flexibility to remove it at some point in time).

Owner

rvagg commented Jan 24, 2013

Have opted for NODE_MODULE_M unless someone comes up with a better idea. I've also changed the docs to point out the differences between the two forms, see here.

I also have the addon examples available in a repo here if anyone is interested in playing them in the current form (as per this branch).

Member

bnoordhuis commented Jan 24, 2013

Have opted for NODE_MODULE_M unless someone comes up with a better idea.

NODE_MODULE_M is better but still a little je n'est ce quoi. Suggestions, anyone?

@bnoordhuis bnoordhuis commented on an outdated diff Jan 24, 2013

doc/api/addons.markdown
-There is no semi-colon after `NODE_MODULE` as it's not a function (see `node.h`).
+There is no semi-colon after `NODE_MODULE_M` as it's not a function (see `node.h`).
@bnoordhuis

bnoordhuis Jan 24, 2013

Member

I'd rather you leave this section untouched. NODE_MODULE is not going away and is what I suspect most people will continue to use.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Jan 24, 2013

doc/api/addons.markdown
@@ -93,6 +97,20 @@ the recently built `hello.node` module:
Please see patterns below for further information or
<https://github.com/arturadib/node-qt> for an example in production.
+### `NODE_MODULE` and `NODE_MODULE_M`
+
+There are two forms of the macro used to register your *init* function, you should use
+only one, depending on your preference or requirements:
+
+* When you register with <code><b>NODE_MODULE</b>(moduleName, registerFunction)</code>,
+your `registerFunction` will be passed the plain <b><code>exports</code></b> object
+when your module is initialized. You will then need to add your properties to this
+object to make them available to client code.
+* When you register with <code><b>NODE_MODULE_M</b>(moduleName, registerFunction)</code>,
+your `registerFunction` will be passed the full <b><code>`module`</code></b> object
+when your module is initialized. This then gives you the ability to completely
+overwrite the `exports` property if you require.
@bnoordhuis

bnoordhuis Jan 24, 2013

Member

Use markdown here, not HTML.

@rvagg

rvagg Jan 24, 2013

Owner

was being cautious as some markdown parsers don't handle nested ` & ** properly.

Owner

rvagg commented Jan 24, 2013

@bnoordhuis see what I mean about nested ` & **; GFM is crap at it but marked is better, which matters most I suppose.

Owner

rvagg commented Jan 24, 2013

hm, no, marked doesn't like that either; it gets the bolded exports and module right, but not the nested bolded NODE_MODULE and NODE_MODULE_M.

Owner

rvagg commented Jan 24, 2013

reverted to html to avoid ` & ** md confusion.

addon: Pass module object to NODE_MODULE init function
mainly to allow native addons to export single functions on
rather than being restricted to operating on an existing
object.

Init functions now receive exports as the first argument, like
before, but also the module object as the second argument, if they
support it.

Related to #4634

cc: @rvagg

isaacs commented Jan 24, 2013

What do you guys think of isaacs/node@ede6592? If we just cast to a function typedef, we can keep the macro the same, and pass in module as the second argument. Existing modules keep working without any changes, and we don't need a new macro name.

Owner

rvagg commented Jan 24, 2013

Nice, if that works cross platform the it's ideal. I'll update docs accordingly when I'm back at a computer.

NODE_MODULE() to pass full `module` to addons
mainly to allow native addons to export single functions on `exports`
rather than being restricted to operating on an existing `exports`
object.

bump NODE_MODULE_VERSION for C++ API breakage
added link to addons repo in docs
Owner

rvagg commented Jan 25, 2013

rebased to @isaacs branch, docs updated, looks good.

isaacs added a commit that referenced this pull request Jan 25, 2013

addon: Pass module object to NODE_MODULE init function
mainly to allow native addons to export single functions on
rather than being restricted to operating on an existing
object.

Init functions now receive exports as the first argument, like
before, but also the module object as the second argument, if they
support it.

Related to #4634

cc: @rvagg

isaacs commented Jan 25, 2013

Landed on master. Thanks @rvagg!

@isaacs isaacs closed this Jan 25, 2013

@jugglinmike jugglinmike referenced this pull request in mozilla-b2g/sockit-to-me Jul 23, 2013

Open

Use NodeJS 'exports' object #5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment