nodejs / nan

Native Abstractions for Node.js
MIT License
3.27k stars 501 forks source link

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

Open tolmasky opened 1 year ago

tolmasky commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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