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

Added NAN_CONSTRUCTOR method. #1

Closed
wants to merge 2 commits into from
Closed

Conversation

kkoopa
Copy link
Collaborator

@kkoopa kkoopa commented Jul 21, 2013

Constructors have no explicit return type and cannot thus be declared the way methods are.

@rvagg
Copy link
Member

rvagg commented Jul 21, 2013

Do you want to have a go at documenting that in the README to show why it might be needed?

My normal pattern is to have a vanilla constructor and do all the fancy V8 extraction work in a static NewInstance that calls it.

Glad to have you on board with this btw!

@kkoopa
Copy link
Collaborator Author

kkoopa commented Jul 21, 2013

Sure, can do -- if this addition really is needed. What I needed it for was when updating node-snappy, which currently has a construct like this:

template<class T> struct SnappyRequest {
  SnappyRequest(const v8::Arguments&);
  std::string input;
  T result;
  v8::Persistent<v8::Function> callback;
  const std::string* err;
};
template<class T> SnappyRequest<T>::SnappyRequest(const v8::Arguments& args){
    ...
}

Minimal amount of rewriting is to make it.

template<class T> struct SnappyRequest {
  NAN_CONSTRUCTOR(SnappyRequest);
  std::string input;
  T result;
  NanCallback *callback;
  const std::string* err;
};
template<class T> NAN_CONSTRUCTOR(SnappyRequest<T>::SnappyRequest) {
    ...
}

@rvagg
Copy link
Member

rvagg commented Jul 21, 2013

Do you think this is a common enough usage? I'm not sure I've seen it before.

If it's an edge case then I wonder if it'd be best to do something like this:

# define _NAN_ARGS const v8::FunctionCallbackInfo<v8::Value>& args
# define NAN_METHOD(name) void name(_NAN_ARGS)

Where _NAN_ARGS could be left undocumented but still available for use in these odd situations. What do you think?

@kkoopa
Copy link
Collaborator Author

kkoopa commented Jul 24, 2013

It might be an edge case, and I might still end up having to rewrite it using NAN's asyncworker class. What I currently have fails on the async part. But yes, your suggestion should work and lead to what is needed, because -- while I haven't seen constructors taking arguments in other projects, I have seen lots of non-callback functions and methods taking the js args as a parameter to do something with them.

@kkoopa kkoopa mentioned this pull request Jul 24, 2013
Closed
@rvagg rvagg closed this Jul 25, 2013
@ghost ghost mentioned this pull request Mar 26, 2016
agnat pushed a commit that referenced this pull request May 6, 2019
zawata pushed a commit to zawata/nan that referenced this pull request Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants