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 2 Discussion #208

Closed
rvagg opened this issue Oct 27, 2014 · 11 comments
Closed

NAN 2 Discussion #208

rvagg opened this issue Oct 27, 2014 · 11 comments
Milestone

Comments

@rvagg
Copy link
Member

rvagg commented Oct 27, 2014

Just a place for putting ideas of things that we might want to break. I don't believe we should invest too much in this and make it a big deal, the more we put in here the longer it'll take and more painful it will be. I think we should just embrace a more rapid release model and not treat a 2.0.0 as a particularly big deal. Regardless, there still needs to be breakage to fix and improve things so put them in here!

Something I'd like us to think about for some point in the future (probably not v2) is making Isolates first-class in NAN so that we're not dealing with a lowest-common-denominator API. Perhaps we introduce a NanIsolate that's a placeholder for pre-0.11 but a real Isolate after that. Then we could require it everywhere V8 needs now but just ignore it in older versions. The point of this would be to make way for code that wants to use Isolates in a more sophisticated way, even though it'd be unlikely you could do that with code that supports pre-0.11 code you'd still be able to use NAN for the additional sugar and helpers and we'd set NAN up better for supporting future versions of V8.

@brett19
Copy link
Contributor

brett19 commented Oct 27, 2014

I think if we are planning to expose Isolates, that we should likely still provide 'isolate free' version of the functions. In the majority of cases, isolates will not be relevant to a node module and I think our API should reflect that. This also means we should be able to actually implement it now without breaking any APIs.

@kkoopa
Copy link
Collaborator

kkoopa commented Oct 27, 2014

Isolates require a clean break with backwards compatibility for older Node versions. I want things to be functionally consistent across versions of Node. If isolates are used it should not even compile on isolate-less versions. It is extremely frustrating to have functionality that does not behave as one might expecte across versions. Having a bunch of caveats like feature X will silently fail on version Y is not sustainable.

tldr: Nan 2 should be 12+ only.

On October 27, 2014 7:20:43 AM EET, Brett Lawson notifications@github.com wrote:

I think if we are planning to expose Isolates, that we should likely
still provide 'isolate free' version of the functions. In the majority
of cases, isolates will not be relevant to a node module and I think
our API should reflect that. This also means we should be able to
actually implement it now without breaking any APIs.


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

@bnoordhuis
Copy link
Member

I think if we are planning to expose Isolates, that we should likely still provide 'isolate free' version of the functions.

V8 plans to move to a fully 'isolate-ified' API without convenience functions like v8::Isolate::GetCurrent() because there won't be a concept of "the active isolate" anymore.

We could try to bolt something on top using thread-local storage but that's probably going to be a very leaky abstraction. Another datum to take into consideration is that node is slowly moving to a multi-isolate model, further muddying the notion of what constitutes the active isolate.

tldr: Nan 2 should be 12+ only.

v0.10 will probably be around for at least another year and that's assuming Joyent gets around to actually making a v0.12 release sometime soon. I'm not holding my breath.

As a discussion starter, the v0.1[012] shim that StrongLoop uses can be found here and here. It assumes that the V8 3.26+ isolate-ified API is truth (and therefore beauty - or was it the other way around?) and exposes that to v0.10 and v0.12 in a consistent manner.

Some upcoming changes that haven't been sync'd to the public repo yet make creative use of SFINAE to paper over the differences between V8 3.26 and 3.29, the versions in joyent/node and node-forward/node respectively.

Long story short, I think it's possible to provide a unified API with only a moderate amount of effort. I suspect that v0.10 will be with us for some time to come and that it is a good idea to keep supporting it for a little while longer.

@kkoopa
Copy link
Collaborator

kkoopa commented Oct 27, 2014

That shim looks nice.

Regarding 0.10 sticking around: If it only were a year... Unfortunately it is going to be around as long as IE 6 (which is probably still around somewhere). For example, I know of too many cases where 0.8 is still being used because it is "battle-hardened and tested" or some other drivel along those lines. Unfortunately, outdated technologies will stick around as long as someone is there making it sort of doable (at best). Some insist on always running ancient software. It's like Debian versus Gentoo, one has old software, the other has new. ;)

The way I see it, NAN 1 offers a way of running legacy code on the current version. NAN 2 could go the other way, offering a way of running modern code on legacy platforms. If you say it is possible to consistently deal with Isolates in legacy versions, then I'm all for it, but more as a fortunate consequence than as a primary goal.

Let's see if some upstream actually will restructure the release process and do away with monolithic releases tied up in themselves for years and years, but if it will continue the same way as now, where 0.12 is going to have a frozen v8 and stick around for 3-4 years, before 0.14 comes and does the same, it will be so much nicer to simply get rid of the baggage going forward. However, it is still too early to tell.

@agnat
Copy link
Collaborator

agnat commented Oct 28, 2014

@kkoopa kindly provided insight and review on agnat/node_mdns#99. I'd like to return the favor.

I'd consider renaming NanReturnValue. The current name suggests that it is a function, which it is obviously not. I'd also like the name to stand out. Maybe something like return_NanValue(...). I think this is much more readable. NanReturnValue is a lot harder to spot when it happens mid-function.

Regarding greedy template functions: This dude scares me a little. In agnat/node_mdns#99 it greedily grabbed an unsigned int and in complicity with implicit conversion miscompiled our code. @kkoopa fixed this already by adding another specialization, which I think is the way to go. You guys already have so many of them that it wouldn't matter to unroll the rest. The greedy, generic version should be replaced with a compile time exception. This would be a non-breaking change.

Last but not least, go to the next C++ store and get you a namespace. I mean look at you! All naked and shit. With all your (greedy template) functions out in the open? Don't you have manners?

Jokes aside, you guys probably have discussed it and decided against it. I just wanted to let you know that as a user of your library I'd like to have it namespaced. In my view namespaces are mandatory in library code.

@bnoordhuis
Copy link
Member

Jokes aside, you guys probably have discussed it and decided against it. I just wanted to let you know that as a user of your library I'd like to have it namespaced. In my view namespaces are mandatory in library code.

I remember bringing that up but I don't know what the conclusion was, if there was one. I agree that leaking symbols into the global namespace is bad.

@kkoopa
Copy link
Collaborator

kkoopa commented Oct 28, 2014

Macros don't know of namespaces, so the point is somewhat moot. What is a macro and what is a function is an implementation detail which can change at any time, the end-user should not be aware of what is what. It's a domain-specific language more than a standard header or library, we need to pull dirty tricks sometimes.

@agnat
Copy link
Collaborator

agnat commented Oct 28, 2014

Hehe... transparent macros. How do you suppose I noticed that NanReturnValue is not a function? Anyway... no namespace. Got it.

How about the return thing? Considering you consider this a DSL shouldn't it be as expressive as possible?

@kkoopa
Copy link
Collaborator

kkoopa commented Oct 28, 2014

Yeah, could at least add an alias for the return thing -- and for 2.0 basically anything goes. NAN started out with nothing but macros and grew from there, there are many legacy artifacts. You are probably right about the greedy template function, it started out with just that one, then came special case after special case. This could actually be done sooner rather than later, as it does not change the (intended) external interface.

I agree that a namespace would be good, but also don't like half-assed implementations. The way it is now we are upfront and honest with symbols leaking. As it cannot currently be fully prevented a namespace might give a false sense of security.

The DSL part is mostly my own twist on things. I work in academia and have to put that sort of spin on what I do. A proper DSL, while "fun" to design, would require additional tools to use and have little to no practical interest for the end-user, among with a subjectively higher learning curve for too narrow a scope. KISS is the ultimate motto in the end. node-cbind takes the DSL concept further in an interesting direction, using NAN as a base.

Oh yeah, speaking of macros and those templates, someone should write a macro or two to spit out those template specializations, it would make things more legible.

@mikeal
Copy link
Contributor

mikeal commented Oct 31, 2014

Pulling in @voodootikigod and @apaprocki who probably care about this kinda stuff.

@rvagg would it be productive to create a list of native projects that use nan and must be targets for support?

@kkoopa
Copy link
Collaborator

kkoopa commented May 22, 2015

Closing in favor of #334.

@kkoopa kkoopa closed this as completed May 22, 2015
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

6 participants