module: pass v8::Object to linked module initialization function

[kaero]

Fixes: #4756

[indutny]

I wonder if it should be module_name_v instead.

[kaero]

fixed

[indutny]

One nit, otherwise LGTM. Tagged as semver-major, because it will break ABI compatibility with previous versions.

[indutny]

CI: https://ci.nodejs.org/job/node-test-pull-request/1310/

[kaero]

because it will break ABI compatibility with previous versions.

But in the issue #4756 @bnoordhuis said:

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.

It looks like the contradiction. If we decide to mark it as the compatibility break, then i intend to add the commit removing the underscore from process._linkedBinding() method name and add the documentation for the method to the process module and, may be, a bit of doc about building node.js with linked native extensions. Otherwise it must not be marked as semver-major change, as for me.

[indutny]

@kaero I'm +1 on making this public. Since we have a conflict here, I guess it needs @nodejs/ctc resolution.

[kaero]

@indutny any additional actions are required from me? I'm not familiar with the conflict resolution process in the node :\

[rvagg]

I'm not so sure there is a conflict, I'm not reading a definitive statement from @bnoordhuis there.

I'm +1 on the change, i.e. lgtm, the symertry with addon loading is nice. However, on that topic, the only reason we do (exports, module, ..) instead of just (module, ...) was for backward compatibility cause we were originally just passing exports.

I'm also +1 on this being semver-major, this API is in use, as evidenced by this PR obviously. Breaking both API and ABI, even for a "private" (because leading-underscore) is not cool.

@bnoordhuis do you want to clarify your position on the semver nature of this?

/cc @thlorenz

[rvagg]

@kaero leave it with us for now I think, we'll get it resolved and let you know if there's anything you need.

If you want to argue for a particular position on semver-major vs minor vs patch then you're welcome to contribute to the discussion, we expect people to champion their positions if they feel strongly one way or another.

[bnoordhuis]

Semver-major seems right to me. The current behavior is unintentional but better to hold off until the next major version on the off chance that someone relies on it.

[bnoordhuis]

LGTM BTW

[indutny]

HONESTLY SAYING, LGTM TOO

[kaero]

Ok, so, i think to move the discussion about publicity of process._linkedBinding to the separate issue/pr?

[trevnorris]

Should exports_prop be placed on Environment like other strings?

[bnoordhuis]

I'd say no. It's normally unused.

[kaero]

Is there any stoppers to merge this PR? :3

[bnoordhuis]

Let's run the CI one more time, the last one had a Windows buildbot acting up. https://ci.nodejs.org/job/node-test-pull-request/1370/

[kaero]

As i had saw before Jenkins was closed by authorization, CI is ok. Can changes be merged?

[jasnell]

CI is green. Will land :-)

[jasnell]

Landed in 71470a8