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

Conversation

@kkoopa
Copy link
Collaborator

commented Jul 21, 2013

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

@rvagg

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Collaborator Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Collaborator Author

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 referenced this pull request Jul 24, 2013
Closed
@rvagg rvagg closed this Jul 25, 2013
@ghost ghost referenced this pull request Mar 26, 2016
agnat pushed a commit that referenced this pull request May 6, 2019
Update README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.