lib: Symbol.dispose should be enabled with experimental flag

[legendecas]

The TC39 explicit resource management proposal is still at stage 3 and
under active development. It must be enabled with an experimental
flag --experimental-explicit-resource-management. The flag implies
that the V8 option --js-explicit-resource-management to be enabled.

When --experimental-explicit-resource-management flag is not set, an
experimental warning is emitted.

Unconditional global presense of Symbol.dispose can be confusing that
eitehr DisposableStack, AsyncDisposableStack and SuppressedError
are not implemented.

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[ljharb]

Isn't Symbol.dispose already polyfilled tho? What happens to those implementations when the flag isn't provided?

[legendecas]

What happens to those implementations when the flag isn't provided?

All Symbol.dispose methods implemented in node are still at experimental stage, like https://github.com/nodejs/node/blob/main/doc/api/child_process.md#subprocesssymboldispose.

A userland polyfill should be able to polifill Symbol.dispose with a detection on the presence of the field. But Node should not unconditionally polifill the global Symbol.dispose.

[ljharb]

What I mean is, if a flag is required to enable the proposal, then shouldn't the Symbol not exist without the flag?

[legendecas]

With this change, the interal symbol exists on primordial SymbolDispose is not really observable from userland -- except with Symbol.for('nodejs.dispose').

It is there for convinience of implementaion like ChildProcess[SymbolDispose]. But they will not be discoverable with the global Symbol.dispose.

[mcollina]

The fact that we have those symbols available allows for TypeScript to implement this correctly on their side. I don't think having these symbols in place cause any harm

[benjamingr]

Yeah I don't see the vlaue in putting this (after a year of usage) behind a flag and it already falls back gracefully to not polyfilling if Symbol.dispose is already defined by V8.

[legendecas]

The symbol is still here. Like other active developing ECMAScript features needing a --harmony-*/--js-* experimental flag, this should be enabled globally only with a flag.

It gives a false impression that the explicit resource management proposal is shipped in the Node.js without a flag.

[mcollina]

The symbol is still here.

How? Maybe I misunderstood this pr.

---

The reasoning behind adding this in this way was that, even thought the syntax was not there, the symbol could be called by relevant tools/transpilers and we could experiment with this API. All of those API would now be behind a flag, essentially regressing support for this.

I'd be ok to do that if that proposal was not moving forward, but it seems it is moving forward, so it seems a step back in removing these to add it later.

[legendecas]

I think a general principal to experiment an early developing globally accessible APIs/syntax should need a flag, reducing the chance of regression or broadly exposed breaking changes.

I am fine leaving Symbol.dispose as an exception to the rule since it has been exposed for a time. But I think a written rule requiring a flag for experimenting unfinished features should be considered: #54330

[legendecas]

TypeScript or any other transpilers can experiment new ECMAScript features without Node.js unconditional polyfills.

Like the following transpile result, it doesn't need Node.js to unconditionally polyfill a global SuppressedError, which is part of the proposal.

example

"use strict";
var __addDisposableResource = (this && this.__addDisposableResource) || function (env, value, async) {
    if (value !== null && value !== void 0) {
        if (typeof value !== "object" && typeof value !== "function") throw new TypeError("Object expected.");
        var dispose, inner;
        if (async) {
            if (!Symbol.asyncDispose) throw new TypeError("Symbol.asyncDispose is not defined.");
            dispose = value[Symbol.asyncDispose];
        }
        if (dispose === void 0) {
            if (!Symbol.dispose) throw new TypeError("Symbol.dispose is not defined.");
            dispose = value[Symbol.dispose];
            if (async) inner = dispose;
        }
        if (typeof dispose !== "function") throw new TypeError("Object not disposable.");
        if (inner) dispose = function() { try { inner.call(this); } catch (e) { return Promise.reject(e); } };
        env.stack.push({ value: value, dispose: dispose, async: async });
    }
    else if (async) {
        env.stack.push({ async: true });
    }
    return value;
};
var __disposeResources = (this && this.__disposeResources) || (function (SuppressedError) {
    return function (env) {
        function fail(e) {
            env.error = env.hasError ? new SuppressedError(e, env.error, "An error was suppressed during disposal.") : e;
            env.hasError = true;
        }
        function next() {
            while (env.stack.length) {
                var rec = env.stack.pop();
                try {
                    var result = rec.dispose && rec.dispose.call(rec.value);
                    if (rec.async) return Promise.resolve(result).then(next, function(e) { fail(e); return next(); });
                }
                catch (e) {
                    fail(e);
                }
            }
            if (env.hasError) throw env.error;
        }
        return next();
    };
})(typeof SuppressedError === "function" ? SuppressedError : function (error, suppressed, message) {
    var e = new Error(message);
    return e.name = "SuppressedError", e.error = error, e.suppressed = suppressed, e;
});
const foo = {
    [Symbol.dispose]() {
        console.log('dispose');
    }
};
var _;
const env_1 = { stack: [], error: void 0, hasError: false };
try {
    _ = __addDisposableResource(env_1, foo, false);
}
catch (e_1) {
    env_1.error = e_1;
    env_1.hasError = true;
}
finally {
    __disposeResources(env_1);
}

So with a global Symbol.dispose, err instanceof SuppressedError or DisposableStack are not available, making the support of the explicit resource management proposal incomplete, yet unconditionally enabled.

[benjamingr]

So with a global Symbol.dispose, err instanceof SuppressedError is not available, making the support of the explicit resource management proposal incomplete, yet unconditionally enabled.

I'm not sure what the issue is though? It's an experimental feature, it's subject to change, we have several similar situations across Node.js (though no polyfills).

We do not implement the explicit resource management proposal though. We don't have using and such (that's up to V8), we merely implement the hooks/apis to allow third-parties (like TypeScript) to build on top of when they implement the proposal.

I think a general principal to experiment an early developing globally accessible APIs/syntax should need a flag, reducing the chance of regression or broadly exposed breaking changes.

Sure but this isn't much of a globally available API? It's like Matteo said just a hook for third-party tools (like TypeScript) to use Symbol.dispose/asyncDispose on APIs.

I agree when there is breakage potential (like exposing EventTarget globally) we need a flag + a major with the flag + a major with an opt out flag in order to make migration easier for the ecosystem.

But if this proposal changes/breaks we break our (intentionally experimental for over a year) APIs. We can document the implication of an API being experimental better though to make it clear what it means.

[legendecas]

We do not implement the explicit resource management proposal though

Symbol.dispose is part of the explicit resource management proposal. So we do implement the proposal incompletely.

Sure but this isn't much of a globally available API?

It is globally available globalThis.Symbol.dispose unconditionally.

[benjamingr]

@legendecas yes, we implement the minimal amount we need in order to be hookable and allow interoperability and experimentation. We do not implement the syntactic parts.

This is similar to many web APIs where we implement the subset it makes sense for Node to implement in core. The way people have been using this this past year is through tools like TypeScript, it's functionally not usable without a third-party tool until V8 implements it.

It is globally available globalThis.Symbol.dispose unconditionally.

Hence the "much" part :) It's very unlikely (unless I'm not considering something obvious) to conflict with existing user code or cause breakage.

[legendecas]

I think my point is that an unfinished feature is unconditionally exposed on the global. It should be experimented with a flag under active development. Like, all actively developing new ECMAScript features will require experimental flags in V8, and it will be confusing that Node.js only implement the language built-in incompletely but enabled it unconditionally.

This is similar to many web APIs where we implement the subset it makes sense for Node to implement in core

I don't agree ECMAScript features are similar to Web APIs we implement partially by intention. globalThis.Symbol.dispose will implemented by V8 and V8 will need a flag --js-explicit-resource-management to enable the feature, and the polyfill will be removed from Node.js.

This PR didn't remove ChildProcess[SymbolDispose] or any Node.js specific hook APIs. It hides globalThis.Symbol.dispose as it will be hide by V8 since it is still under active development.

[legendecas]

Updated to emit an experimental warning when either --experimental-explicit-resource-management or V8 --js-explicit-resource-management flag is not defined. The Symbol.dipose and Symbol.asyncDipose (equivalent to Symbol.for('nodejs.dispose')) are kept globally available.

The experimental warning will be disabled when either --experimental-explicit-resource-management or V8 --js-explicit-resource-management flag is defined.

@mcollina @benjamingr

[benjamingr]

I don't think it's good UX to add an experimental warning a year into it, but if this was a new feature I would have been fine with an experimental warning.

[codecov]

Codecov Report

Attention: Patch coverage is 88.23529% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.04%. Comparing base (88bac52) to head (8accb84).
Report is 165 commits behind head on main.

Files	Patch %	Lines
lib/internal/process/pre_execution.js	86.36%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54329      +/-   ##
==========================================
- Coverage   87.11%   87.04%   -0.07%     
==========================================
  Files         647      647              
  Lines      181754   181957     +203     
  Branches    34885    34895      +10     
==========================================
+ Hits       158332   158393      +61     
- Misses      16738    16861     +123     
- Partials     6684     6703      +19     

Files	Coverage Δ
src/node_options.cc	88.23% <100.00%> (+0.14%)	⬆️
src/node_options.h	98.18% <100.00%> (+0.02%)	⬆️
lib/internal/process/pre_execution.js	90.43% <86.36%> (-0.50%)	⬇️

... and 45 files with indirect coverage changes

[mcollina]

LGTM, we should likely have been emitting a warning from the beginning for this feature.

Can you please add a test that uses the using feature?

[benjamingr]

Can you please add a test that uses the using feature?

As in with --experimental-strip-types?

[mcollina]

My understanding is that --js-explicit-resource-management does actually enable the syntax.

[legendecas]

My understanding is that --js-explicit-resource-management does actually enable the syntax.

The flag is present but the implementation is unfinished. The syntax support is still limited and not functional on the node.js main branch.

[aduh95]

It'd be nice if passing --no-experimental--explicit-resource-management would result in Symbol.dispose to not be created by us – but I doubt it's possible with our parser for boolean flags

[legendecas]

It'd be nice if passing --no-experimental--explicit-resource-management would result in Symbol.dispose to not be created by us – but I doubt it's possible with our parser for boolean flags

Right, boolean flags are not possible to be distinguished whether a --no-* is passed with getOptionValue as they are added automatically.

[benjamingr]

I still think we shouldn't do this a year into it (for developer expectations/ux concerns), but I think in retrospect it should have had an experimental warning and we should change the process to require TSC consensus before landing new globals (especially experimental ones) since that's a cross cutting concern.

[benjamingr]

Also at the very least it should have been semver-major by default without other considerations since it added a new global

[mcollina]

I can stand by the ship being sailed on emitting the experimental warning on this one too.

@nodejs/tsc what do you all think?

[MoLow]

I don't think this should land as is. It will break existing users.
We can add a warning, but making it opt-in should be samver major.

[legendecas]

@MoLow the PR only emits experimental warnings. Enable flag --experimental-explicit-resource-management will disable the warning.

[benjamingr]

Let's not focus on this particular feature (Symbol.dispose) can we get TSC consensus/evaluation of safety on requiring TSC consensus for polyfilling/adding experimental JS APIs with (even tiny) potential breakage potential?

[legendecas]

Closed in favor of #54330.