0.8.1-wip

[rvagg]

No description provided.

[bnoordhuis]

If you opened this PR in order to get a LGTM then LGTM. :-)

[rvagg]

heh, no, just to bundle stuff up for an 0.8.1, just fixing the tests didn't seem enough for an actual release.

[kkoopa]

This should be enough for releasing 0.8.1, but I think 0.9.0 would be more appropriate.

[rvagg]

@kkoopa given the type arg stuff in particular, this is probably a 0.9 release; agree?

[kkoopa]

Yes it is. Documentation still needs updating. However, I have not been able to find out how the new weak callbacks differ from the old. In the past you would commonly call Persistent::MakeWeak again from within the callback. This is no longer possible with the new weak callbacks, as Persistent::MakeWeak requires a Persistent, which does not seem available any more.

[kkoopa]

I think Persistent::MakeWeak is gone in v8 3.24, so using the old way would only be a temporary fix until the next version of Node shipped with updated V8. It would be better to resolve this properly and proactively. But there is no documentation or anything at all. The documentation for old weak callbacks stated that the callback had to either dispose of the persistent handle or revive the handle by calling Persistent::MakeWeak on it again. This cannot be done with the new style using Persistent::SetWeak as the persistent handle is not available any more, is it safe to assume it can be ignored then?

This also means we would have to introduce a NanRevivePersistentInCallback and NanDisposePersistentInCallback which would be no-ops on new versions.

[rvagg]

I've asked TJ for a new 0.11 release, I'll push again this week. There seems to be a focus on 0.12 atm, which is unfortunate, plus everyone's distracted with all the AsyncListener changes. What we really need is a new release that fixes the 0.11.11 problems and upgrades V8 so we can test and fix.

[kkoopa]

Yes, that would be most appropriate. I think I know how the persistent stuff is to be done now. https://github.com/indutny/node/blob/feature/update-v8-3-24/src/smalloc.cc uses a CallbackInfo struct containing the persistent handle, which gets passed around.

[kkoopa]

Yeah, it compiles at least. But, there is a whole lot of boilerplate. This could quite easily be reduced and hidden away in NAN.

[kkoopa]

But this would leak the original Persistent now, wouldn't it?

[kkoopa]

It would be possible to construct something like this:
https://github.com/kkoopa/nan/blob/possible_api/nan.h#L365-L394
giving the following use:
https://github.com/kkoopa/nan/blob/possible_api/test/cpp/weak.cpp#L11-L27

[rvagg]

I think that'd be ok, at least it looks NANish, I can't speak to the specifics because I don't have use for weak callbacks (yet). If you can get this happily working with old and new v8 cross-platform then 👍 from me!

[kkoopa]

Should the Dispose be left as-is or should the implementation be left to the user, only providing a virtual interface, similar to the async class?

[rvagg]

ahhhh.. I'm really not sure, I have about 100 things going on at the moment so I'm not able to think clearly about any of them. I'd say to start simple and complicate later if you run into real problems when you start using it in real projects. Bumping minor isn't a big deal if we add new stuff.

[kkoopa]

0.11.12 is out now