async_hooks: rename AsyncLocalStore#enterWith to set

[Flarna]

The name enterWith indicates that there should be a matching exit but this is not the case. AsyncLocalStore#exit exists but it is not the opponent of enterWith.

enterWith() is kept as alias and marked as doc only deprecated.

In special during review of #45277 I found it very confusing the we do enterWith in start and exit events.

@nodejs/diagnostics @vdeturckheim

[Qard]

I actually originally called that function set(...) but there was endless bike shedding around the name as people felt the set(...) naming was confusing too. In any case, I'm good with whatever we want to call it, but my intent with my recent work with diagnostics_channel is to (hopefully) render it as a public interface obsolete. Ideally we should be able to make it internal-only, if my efforts succeed.

[anonrig]

Can you emit warning when enterWith is called?

[vdeturckheim]

Util.deprecate should do the trick

[Flarna]

Is there a reason for not using a doc only deprecation here - at least for a while? Deprecation logs may break apps using the console output.

It's just a about an inoptimum naming not a functional problem which results in some risk/problem.

[vdeturckheim]

I'd argue we want the deprecation logs to show for a few versions before we remove enterWith . Alternativelly, we can keep enterWith and doc it as an alias for set, but if we want it out, we need to make sure we never break code with . enterWith is not a function

[Flarna]

I'm fine with keeping it as documented alias like I did it here. My main intention is that looking into real world use cases as in the above linked PR the name looks better to me and results in easier to read code.

But enterWith is not that bad that I would force people to change. can't tell how many users there are actually.

[targos]

Maybe an advantage of enterWith is that it's "unique" and we can easily search for its usage in GitHub and other open source databases.

[vdeturckheim]

As mentioned in the main thread, I changed my mind, I think we should keep enterWith as a forever alias due to its use in existing APMs and we don't want to break any of their customers on whatever version of node x version of the APM.

[Qard]

Yeah, this is used extensively at Datadog currently so I definitely would not want to go straight to warning deprecation. We should have a doc deprecation phase, an eventual warning phase, and then maybe removal.

As I stated further up in the issue though, I'm working on making this API (hopefully) obsolete so not sure changes the this will really matter much.

[aduh95]

I don't know if we have a precedent of deprecating an experimental API, typically we would simply remove it. I don't know that API, so maybe what I'm saying doesn't make sense, let me know, but what I would suggest would be to introduce set as stable, and to add an experimental warning to entryWith suggesting to use .set instead (otherwise I don't see why users would want to migrate from an experimental API to another experimental API).

[vdeturckheim]

tbh, I am not a big fan of set as a name for this method (without saying that enterWith is a good name either). Calling this method has a side effect on the running process and in my understanding (but I am not a native English speaker) set is a verb that does not carry such meaning. Calling set on an instance of Map should only set a value on the object it is called on. To me, in the case of AsyncLocalStorage this is not a set method as much as a set-and-start-propagation one.

That said, we have AsyncLocalStorage in core as partially stable and the stable parts are safe enough to use. This is the most important imho so I won't fight such a name change.

Regarding current use of enterWith it seems it's used around and keeping it as an alias forever is probably the safest thing without having a big cost.

[Flarna]

As set is still not seen better as enterWith in general and considering the friction this may generate I go ahead and close this one. If we succeed to remove the API it's anyway don't care how it was named, if not we can still add an alias later.