errors: should we raise assersions for type checks in process.bindings?

[joyeecheung]

To migrate the errors thrown in C++ land we currently try to collect the context in C++ then throw the actual errors in JS using utilities in internal/errors.js. The current pattern would replace code like

if (!args.Length() < 1) {
   env->ThrowError(...)
}

with

CHECK(args.Length() >= 1);

and enforce the type checks with ->To*(context).ToLocalChecked().

The JS layer would do the checks in advance and make sure the bindings are invoked correctly, but if there are people using process.bindings() with wrong types of objects, previously they got errors, now they would get assersions.

This issues is to make sure we are OK with this path forward. Otherwise we need to somehow port the stuff in internal/errors.js back to C++ land for migrating those errors.

[joyeecheung]

cc @nodejs/tsc @jasnell

[targos]

I'm OK with this.

[fhinkel]

I'm ok with this. Nit: there's a CHECK_GE(length, 1) macro we should use.

[mhdawson]

I'm generally ok, the only concern is if this will cause significant breakage for existing users. I guess though that in either case they would have been doing something incorrect and my first guess is that there should not be too much code catching the error and just continuing.

[MylesBorins]

Would it make sense to do an initial naive implementation and run that against citgm?

[joyeecheung]

@MylesBorins I think #17334 pretty much handles most of the type check migration

[mcollina]

I am 👍 with the approach, independently off the breakage.

[joyeecheung]

This can be closed now since #17334 has landed