Add function and object template factories

[agnat]

This popped up while working on #243.

Ready for review.

[kkoopa]

What about the 4-arg FunctionTemplate with the final length argument?

[agnat]

Right... I probably was looking at an old v8 header and must have missed it. Will add it later today...

[kkoopa]

If it wasn't always available we can't really add it. Lowest common denominator and all. I just assumed it was.

On January 19, 2015 11:17:51 AM EET, David Siegel [email protected] wrote:

Right... I probably was looking at an old v8 header and must have
missed it. Will add it later today...

---

Reply to this email directly or view it on GitHub:
#248 (comment)

[agnat]

Hm, I thought about that when implementing Factory<Function>. (Direct construction of Function is only available in newer v8.) I think just offering the LCD is not going to cut it in the long run. A project might target node >=0.10 or io.js. It still wants NaNs future proofing even (or specially) for features that are not part of the LCD (yet). I think a good approach is to wrap new features early and if possible provide graceful degradation on older versions. We already do that for (bound) scripts. If we can't reasonably emulate a feature so be it. It just needs to be documented.

Just my opinion, though.

[kkoopa]

Yes, where there is not too much work, backporting and emulating is all very good. Just want the external interface to be identical.

I think that something correctly written in proper NAN should work on every supported version. It should not be that parts only work on certain versions.

For that we should have new major versions of NAN that drop support for older nodes and break the API.

i.e. NAN 1 is for 0.8+, NAN 2 for 0.10+, NAN 3 for 0.12+. But nothing is of course set in stone and I don't know how v8 will evolve in the future, this needs to be adapted to reality. io.js claims to update v8 more often, so maybe we have to follow that. Who knows...

See e.g. #208

On January 19, 2015 12:58:53 PM EET, David Siegel [email protected] wrote:

Hm, I thought about that when implementing Factory<Function>. (Direct
construction of Function is only available in newer v8.) I think just
offering the LCD is not going to cut it in the long run. A project
might target node >=0.10 or io.js. It still wants NaNs future proofing
even (or specially) for features that are not part of the LCD (yet). I
think a good approach is to wrap new features early and if possible
provide graceful degradation on older versions. We already do that for
(bound) scripts. If we can't reasonably emulate a feature so be it. It
just needs to be documented.

Just my opinion, though.

---

Reply to this email directly or view it on GitHub:
#248 (comment)

[kkoopa]

Can't write a test for NanNew<Function> without doing it conditionally on version. Perhaps a conditional define would be the solution? Build with -DNAN_BACKWARDS_COMPATIBLE 1337 or something to get it enabled.

[agnat]

On Jan 19, 2015, at 15:51, Benjamin Byholm [email protected] wrote:

Can't write a test for NanNew without doing it conditionally on version.

Just backport the constructor. It just creates a function template and returns the resulting function. I already checked. I can do it tonight...

The length argument requires more doing, though.

Perhaps a conditional define would be the solution? Build with -DNAN_BACKWARDS_COMPATIBLE 1337 or something to get it enabled.

Opt-in seems like the right thing to do (in general). Either with defines or by including additional/different headers.

—
Reply to this email directly or view it on GitHub.

[kkoopa]

If backporting works, that would be great. From what I have seen, it is usually impossible, as anything that uses v8 "internal" stuff needs to be in the correct classes, and they are all non-extendable. But, if it just does a FunctionTemplate and returns the function, acting like a convenience method, that should indeed work!

Length argument might be impossible for above mentioned reasons. I'll take a quick glance at the code and see.

I like opt-in too. As explicit as possible, that should guard against spurious bug reports for "broken" functionality. Still requires good documentation (better organized than what we currently have).

[kkoopa]

Length can be done. It ultimately just does obj->set_length(length);, which can be done externally through FunctionTemplate::SetLength. So, just construct a function template and do SetLength on it before returning.

[agnat]

Unfortunately FunctionTemplate::SetLength() is a new v8 feature, too. Guess, we have to skip the length argument for now... :-/

[kkoopa]

Crap. Maybe it's possible to use
Object::ForceSet(NanNew("length"), NanNew(length), v8::ReadOnly);
or similar?

@bnoordhuis: Any suggestions?

On Monday 19 January 2015 17:32:17 David Siegel wrote:

Unfortunately FunctionTemplate::SetLength() is a new v8 feature, too.
Guess, we have to skip the length argument for now... :-/

---

Reply to this email directly or view it on GitHub:
#248 (comment)

[kkoopa]

I guess it is new because it is part of the ES6 standard.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-functioninitialize

Let status be DefinePropertyOrThrow(F, "length", PropertyDescriptor{[[Value]]:
len, [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: true}).

On Monday 19 January 2015 17:32:17 David Siegel wrote:

Unfortunately FunctionTemplate::SetLength() is a new v8 feature, too.
Guess, we have to skip the length argument for now... :-/

---

Reply to this email directly or view it on GitHub:
#248 (comment)

[kkoopa]

The actual v8 implementation seems to define the property through an accessor pair operating on an internal field of sorts at kLengthOffset.

[kkoopa]

I want to get this merged so I can push the new versions.

Waiting for backport of Function::New.

Unsure about whether FunctionTemplate::New with length argument is doable, can hold off until later.

[agnat]

Ack. Will finish it tonight.

[agnat]

I found no (good) way to back port Function.length. #define NAN_ENABLE_FUNCTION_LENGTH enables the additional argument. Not sure I like it, though. Maybe we should keep it under the wraps for a bit and think about it some more.

Ready for review.

[kkoopa]

I think the extra length argument is quite messy, but I can't see a better way of doing it either. I agree with keeping it under the wraps until some future point, preferrably not having it clutter up the code for now. If absolutely necessary at some future point, we can add it in -- provided that no fully compatible solution shows up.

[agnat]

I removed the length argument for now. I'll add another PR to discuss our options.

[kkoopa]

Excellent. I'll merge this, do some testing and push out the new releases then.