Fix v8::External::Value/New for the new ExternalPointerTypeTag API

[gdavidkov]

Problem

V8 added an ExternalPointerTypeTag parameter to v8::External::New and v8::External::Value and removed the prior overloads:

static Local<External> New(Isolate*, void*, ExternalPointerTypeTag);
void* Value(ExternalPointerTypeTag) const;

The new signatures are gated by V8's external-pointer-tagging build configuration, signaled by the public macro V8_EXTERNAL_POINTER_TAG_COUNT. A V8_MAJOR_VERSION check is unreliable here because Node and Chromium ship divergent V8 builds — the feature can be enabled in Electron's V8 and not in upstream Node's V8 at the same nominal major version.

nan still calls the removed forms — 28 sites in nan_callbacks_12_inl.h (the imp::*CallbackWrapper trampolines) and 3 in nan_implementation_12_inl.h (the External / Function / FunctionTemplate factories). Both are header-inline, so any addon that includes <nan.h> fails to build against a tag-enabled V8. Reproduced against Electron 42.0.0-beta.8 (Node 24.15.0) on Windows + MSVC 2022:

nan_callbacks_12_inl.h(189): error C2664:
  cannot convert from 'v8::Isolate *' to 'v8::ExternalPointerTypeTag'
nan_implementation_12_inl.h(79): error C2660:
  'v8::External::New' does not take 2 arguments

Fix

Two private inline helpers in the existing imp:: namespace, one per file next to its callers:

inline void* GetExternalPointer(v8::Local<v8::External> ext);
inline v8::Local<v8::External> NewExternal(v8::Isolate*, void*);

Each selects the tag-taking signature when V8_EXTERNAL_POINTER_TAG_COUNT is defined, passing v8::kExternalPointerTypeTagDefault (V8's no-tagging default for embedders that do not partition externals by type), and falls back to the legacy form otherwise. All 31 callsites are routed through the helpers. Both use the same tag, so create/read tags match by construction.

Backwards compatibility

Every change is gated on #ifdef V8_EXTERNAL_POINTER_TAG_COUNT. The #else branches reproduce the prior code byte-for-byte — no-op on every V8 build that doesn't define the macro.

Tests

test/cpp/news.cpp::NewExternal and test/cpp/nannew.cpp::testExternal round-trip a void* through Nan::New<v8::External> and read it back via raw v8::External::Value(). They hit the same compile break as nan's headers; the same gate is applied at each test site. NewExternal exercises imp::NewExternal directly; imp::GetExternalPointer is exercised every time any NAN_METHOD test runs from JS, since imp::FunctionCallbackWrapper unpacks the registered callback through it before dispatch.

Out of scope

V8 14 work already landed (#1010, #1013); this PR addresses only the External API change. nan_callbacks_pre_12_inl.h / nan_implementation_pre_12_inl.h target Node ≤ 0.12 (pre-V8-3.x), which uses a still-older single-arg form, so a tag-enabled V8 never selects those files.

Verified

Builds clean against Electron 42.0.0-beta.8 (Node 24.15.0) on Windows + MSVC 2022. CI will exercise both branches across the existing matrix.

[agracio]

nan Github CI no longer runs on Node 16 and 17.
This RP addresses issues with v8 14.8.

[kkoopa]

I tried looking at the CI output, but do not understand why 16 and 17 fail, yet the others work.

[kkoopa]

There should not be a need to (manually) do any preprocessor conditionals in the tests, since that would require doing so in actual code. It has been a long time, so I do not remember all the internals anymore, but it seems like v8::External needs to be duplicated by a nan::External, exposing the current (old) API and internally sticking on a tag when needed. One could possibly do it with free functions, Nan::New for the constructor and a new function NanGetExternal or something for getting the value, but that seems very ugly. What are your thoughts on this?

[gdavidkov]

You're right that the #ifdefs in the tests are a smell — if the tests need them, addon code needs them too, and that's exactly what NAN should be hiding. Thanks for pushing back on that.

I see three plausible directions, with different tradeoffs:

1. Public read-side helper. The current PR already routes Nan::Newv8::External(value) through an imp::NewExternal helper that applies the tag, so the write side is already abstracted. The only remaining gap on the public surface is ->Value() — there's no factory analog for the read. Promoting the second helper from this PR to something like Nan::ExternalValue(local) would close that gap, and the tests drop their #ifdefs. NAN already uses this exact inline-helper pattern internally for the analogous v8::String::GetExternal* / IsExternal* renames (in imp::), so the mechanism isn't novel — this would just expose it publicly. Smallest diff, fully backwards compatible. You called this "very ugly" though, and I take the point: it leaves v8::Localv8::External as the carried type, so users still have to remember to call Nan::ExternalValue(e) instead of e->Value().
2. Nan::External wrapper class along the lines you described — a thin value type holding a v8::Localv8::External, exposing the old New(value) / Value() shape and internally adding the tag on V8 ≥ 13. Cleanest from a user-migration standpoint (literal v8::External → Nan::External find-and-replace), but a couple of constraints worth flagging:
   • We can't change the return type of the existing Factoryv8::External::New (Nan::Newv8::External returning v8::Localv8::External) without breaking every addon that already assigns the result to a v8::Localv8::External. So Nan::Newv8::External stays as-is for compat, and Nan::NewNan::External becomes the new opt-in path via a FactoryNan::External specialization — two parallel factories.
   • The wrapper doesn't cover externals that don't come from Nan::New — e.g., info[0].Asv8::External() from JS-passed arguments, or obj->GetInternalField(i).Asv8::External() inside our own callback wrappers. Those callers already hold a raw v8::Localv8::External and need either the free helper from (1) or a Nan::External(rawLocal).Value() constructor path to read it tagged.
3. Hybrid — both of the above. Public read helper as the underlying mechanism (covers raw-Local sites, including NAN's own callback wrappers), and Nan::External as an optional sugar wrapper for users who want the drop-in feel. This is what I'd lean toward — strict superset of (1) and (2), and avoids choosing between "covers all sites" and "ergonomic drop-in."

Sketch of the wrapper if it helps anchor the discussion:

namespace Nan {
class External {
  v8::Local<v8::External> handle_;
 public:
  External() = default;
  explicit External(v8::Local<v8::External> h) : handle_(h) {}

  static External New(void* value) {
    v8::Isolate* iso = v8::Isolate::GetCurrent();
#ifdef V8_EXTERNAL_POINTER_TAG_COUNT
    return External(v8::External::New(iso, value,
                                      v8::kExternalPointerTypeTagDefault));
#else
    return External(v8::External::New(iso, value));
#endif
  }

  void* Value() const {
#ifdef V8_EXTERNAL_POINTER_TAG_COUNT
    return handle_->Value(v8::kExternalPointerTypeTagDefault);
#else
    return handle_->Value();
#endif
  }

  bool IsEmpty() const { return handle_.IsEmpty(); }
  operator v8::Local<v8::External>() const { return handle_; }
};
}

Isolate is resolved via v8::Isolate::GetCurrent() to match the existing Factoryv8::External::New. Only the conversion to v8::Localv8::External is needed — Local already converts to Local in V8, so a second operator would just invite overload ambiguity. Call site is e.Value() (dot, not arrow), which matches Nan::Callback / Nan::Persistent style.

Happy to take this PR in whichever direction you prefer. If you're good with the hybrid, I'll drop the #ifdefs in the tests, add the public helper + wrapper, and route the internal callback code through them.

[kkoopa]

Yes, that wrapper class looks exactly like what I had in mind, but your second point does list genuine constraints I had not thought of, making the wrapper class approach somewhat less nice. Looking through the API documentation, I see a lot of free functions working on v8 types, so free functions one must remember to use are not at all uncommon. I made it like that, I still think it is ugly, but I had forgotten how much ugliness there already was. Thinking back, the use of free functions probably stemmed from saving effort on wrapping entire classes. I am therefore now leaning towards option 1 over option 2. Option 3 sounds interesting if you think it would be genuinely useful to have a sugar class.

I do not know how many addons explicitly use Externals, but I believe there should not be that many call sites to e.Value() in total and the compiler will catch them all, so at least there is no risk of runtime errors. The documentation would also need updating (edit individual files under doc/, then run make docs to update README.md).