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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

kaero
Copy link
Contributor

@kaero kaero commented Jan 19, 2016

Fixes: #4756

@Fishrock123 Fishrock123 added c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. labels Jan 19, 2016
indutny
indutny reviewed Jan 20, 2016
View reviewed changes
src/node.cc

if (exports_v->IsObject())
return args.GetReturnValue().Set(exports_v.As<Object>());

node::Utf8Value module_v(env->isolate(), module);
node::Utf8Value module_v(env->isolate(), module_name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it should be module_name_v instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@indutny indutny added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 20, 2016
@indutny
Copy link
Member

indutny commented Jan 20, 2016

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

@indutny
Copy link
Member

indutny commented Jan 20, 2016

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

@kaero
Copy link
Contributor Author

kaero commented Jan 20, 2016

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

indutny commented Jan 20, 2016

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

@kaero
Copy link
Contributor Author

kaero commented Jan 20, 2016

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

@rvagg
Copy link
Member

rvagg commented Jan 20, 2016

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

rvagg commented Jan 20, 2016

@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
Copy link
Member

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

LGTM BTW

@indutny
Copy link
Member

indutny commented Jan 20, 2016

HONESTLY SAYING, LGTM TOO

@kaero
Copy link
Contributor Author

kaero commented Jan 20, 2016

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

trevnorris
trevnorris reviewed Jan 20, 2016
View reviewed changes
src/node.cc
Local<Object> exports = Object::New(env->isolate());
Local<String> exports_prop = String::NewFromUtf8(env->isolate(), "exports");
module->Set(exports_prop, exports);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should exports_prop be placed on Environment like other strings?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kaero
Copy link
Contributor Author

kaero commented Jan 25, 2016

Is there any stoppers to merge this PR? :3

@bnoordhuis
Copy link
Member

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

kaero commented Feb 2, 2016

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

@jasnell
Copy link
Member

jasnell commented Feb 2, 2016

CI is green. Will land :-)

jasnell pushed a commit that referenced this pull request Feb 2, 2016
@jasnell
module: pass v8::Object to linked module initialization function
71470a8 
Fixes: #4756
PR-URL: #4771
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@jasnell
Copy link
Member

jasnell commented Feb 2, 2016

Landed in 71470a8

@jasnell jasnell closed this Feb 2, 2016
@kaero kaero mentioned this pull request Feb 20, 2016
Closed
@jasnell jasnell mentioned this pull request Mar 17, 2016
Closed
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
module: pass v8::Object to linked module initialization function
986b166 
Fixes: nodejs#4756
PR-URL: nodejs#4771
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kaero @indutny @rvagg @bnoordhuis @jasnell @trevnorris @Fishrock123