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

Nan::SetProperty (const char * version of Nan::Set to match Nan::SetMethod) #947

Open
tolmasky opened this issue Jan 13, 2023 · 4 comments

Comments

@tolmasky
Copy link

Perhaps I missed it, but there does not appear to be a convenience method that lets you just pass a C string for the property name to Nan::Set like you can do with Nan::SetMethod. Perhaps there is a reason for this, but I always end up making a macro that does this for myself and figured it was worth asking if it would be fine to add officially here, either as an overloaded version of Nan::Set or as a new Nan::SetProperty method. If so I can make a PR.

@kkoopa
Copy link
Collaborator

kkoopa commented Jan 13, 2023

The main reason why it does not exist is because the function it papers over does not have an overload accepting a const char *. While NAN does contain some convenience methods, the primary reason is just for source-level version independence. Now, for this particular feature, I am not opposed to adding it, since it seems fairly small in scope at first glance, but I have a faint recollection that polymorphism involving char * in this way may have some nasty edge cases under certain conditions, but I do not know if they would apply here, nor if I remember correctly. It has been years I last had to deal with these things, so forgive my rusty memory.

That being said, if you were to submit a PR adding this feature and it provably does not break anything, I would gladly merge it.

@tolmasky
Copy link
Author

Would you prefer I create a new named method in that case, like SetProperty? I think this would ensure we don't hit any polymorphism issues since it wouldn't be overloading Nan::Set. Otherwise I am happy to do it with Nan::Set too, whichever you prefer.

@kkoopa
Copy link
Collaborator

kkoopa commented Jan 13, 2023

If it were to be added, I would prefer it to be polymorphic (through overload or template, whichever is best for this). I had a quick glance at the code and there are all these related functions with key:value like Nan::Get, Nan:DefineOwnProperty etc. that should receive the same treatment for consistency's sake, so the scope already grew a bit. I would like to hear from our resident C++ expert @agnat, if he has time to give an opinion. If a simple set of overloads will do, that is probably the easiest solution.

@agnat
Copy link
Collaborator

agnat commented Jan 14, 2023

At a glance I think an overload should be fine...

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

No branches or pull requests

3 participants