doc: deprecate coercion to integer in process.exit()

[daeyeon]

Refs: #43716

This warns of invalid uses of process.exit([code]) (such as in the comment)
and recommends the correct practice.

The code passed as the first argument to process.reallyExit() is
converted to an integer. This behavior has been unofficially supported so far.

node/src/node_process_methods.cc

Lines 442 to 447 in adef64c

	static void ReallyExit(const FunctionCallbackInfo<Value>& args) {
	Environment* env = Environment::GetCurrent(args);
	RunAtExit(env);
	int code = args[0]->Int32Value(env->context()).FromMaybe(0);
	env->Exit(code);
	}

Signed-off-by: Daeyeon Jeong [email protected]

[bnoordhuis]

This seems like too trivial a thing to bother users with. The only thing I'd maybe change is FromMaybe(0) to e.g. FromMaybe(1) - a zero exit code when coercion fails is arguably misleading.

[aduh95]

This seems like too trivial a thing to bother users with.

I'd argue that's exactly what doc-only deprecations are for.

The only thing I'd maybe change is FromMaybe(0) to e.g. FromMaybe(1) - a zero exit code when coercion fails is arguably misleading.

Neat idea, but that would still need to go through a deprecation cycle, right?

[daeyeon]

This seems like too trivial a thing to bother users with. The only thing I'd maybe change is FromMaybe(0) to e.g. FromMaybe(1) - a zero exit code when coercion fails is arguably misleading.

I fully understand that opinion. No matter what code is passed, users will terminate node as expected. So this may be trivial to both users and us. One case I guessed was that there could be a few users wondering why the code they passed isn't correctly read after terminating node with codes like -1 or 'end'. So I wondered what others think of this issue. However, I didn't expect how many undocumented wrong uses we should consider.

I actually started this PR to refactor process.exitCode with getter/setter accessors. In the following case, we need to read the JS property multiple times to sync it on the native side. It seemed to be worth refactoring since we could reduce unnecessary calls. (It's a todo created by @addaleax)

node/src/api/hooks.cc

Lines 77 to 87 in 3d575a4

	Local<String> exit_code = env->exit_code_string();
	Local<Value> code_v;
	int code;
	if (!process_object->Get(context, exit_code).ToLocal(&code_v) ||
	!code_v->Int32Value(context).To(&code) ||
	ProcessEmit(env, "exit", Integer::New(isolate, code)).IsEmpty() ||
	// Reload exit code, it may be changed by `emit('exit')`
	!process_object->Get(context, exit_code).ToLocal(&code_v) ||
	!code_v->Int32Value(context).To(&code)) {
	return Nothing<int>();
	}

However, I couldn't promptly get a clear answer on what types the accessors can accept because of the loose validation. That was a problem I had.

Thanks for the opinion. I'm happily learning many from all.

[RaisinTen]

LGTM

[aduh95]

@daeyeon Can you please rebase to fix the conflict?

[daeyeon]

@aduh95 Rebased, thanks!

[nodejs-github-bot]

Landed in b328581