Are non-undefined primitives needed as unregistration tokens?

[marjakh]

See discussion starting at https://bugs.chromium.org/p/v8/issues/detail?id=8179#c24

Afaik the current spec doesn't match the intention.

[littledan]

Should the spec check for undefined as the unregister token, or check for any non-object? I don't see a good reason to omit other primitives, but there was some past discussion about treating all primitives as "no unregister token".

[mhofman]

A number or string seems handy as a token. The token as a number could be the index in an array kept by the user for example.

I can actually see some use cases where the user doesn't care about the token at all, and when calling unregister without it, it would simply unregister all cells without a specific token (i.e. the way it's currently specd) .
There is a risk that it's not quite obvious or usual. I'm actually not familiar with other APIs using tokens like this. What are the precedents or best practices?

[mhofman]

If we're to not allow undefined as registration tokens, does it make sense then to automatically use holding as the token if none (undefined) is provided?

I keep finding myself writing finalizationGroup.register(target, info, info);

[erights]

When we came up with this API, we expected that fg.register(target, info, info) would be a common pattern. But the uses of registration token that do not fall into this pattern are still necessary and useful. In particular, omitting the token in order not to register.

Using undefined as the registration token should not cause unregistration. Rather, it should act exactly as omitting this argument would: it would fail to cause a registration, but would not affect any existing registration.

[mhofman]

Using undefined as the registration token should not cause unregistration. Rather, it should act exactly as omitting this argument would: it would fail to cause a registration, but would not affect any existing registration.

Sorry, not sure I follow. What is register(target, info) supposed to do? It should create a cell, right?
I guess you mean the user wouldn't be able to un-register such a holding?

[erights]

I guess you mean the user wouldn't be able to un-register such a holding?

Exactly. This will be a frequent case, and we designed the API so that this case didn't need to pay for bookkeeping it does not need.

By contrast, a frequent pattern for reified ability to unregister in other APIs, for example Observable, is to return a function (or object with method) that one can call to unregister. This makes all unregisterable operations pay for bookkeeping that is rarely needed. If we had thought of it at the time, we should have used the registration token pattern there as well.

[littledan]

@erights I agree with you. A remaining question in my mind is, should we permit something like a Number to be an unregistration token?

[zenparsing]

If we had thought of it at the time, we should have used the registration token pattern there (Observable) as well.

I'm not so sure. The compositional nature of the API was fairly well tied up with the return value, and it would have been argued for on ergonomic grounds anyway.

I wonder if this is another case where we would have benefited from a language-level generalized cancel token API, instead of the platform-specific AbortController thing we have.

[mhofman]

I understand the wish for being able to optimize the case when user don't need to unregister. However right now I'm a little concerned about the ergonomics as a developer.

First, there's not really a precedent for a registration token passed to an API that's different than the thing being registered.
Writing the tests I've actually found myself writing register(target, info) instead of register(target, info, info) a surprising amount of times. Since I was writing tests, it got caught quickly, but I wouldn't be so sure for regular code.
The problem is that the mistake is sneaky. Editors will not warn you about it at all since the token is an optional parameter. Calling unregister with a token that doesn't exist will also not really warn you since most people don't check return values. And there are genuine code paths for the unregister method returning false, aka calling unregister after the target was collected.

Second, I'm actually not sure about the assumption that users may not want to use unregister most of the time. After all, this API is about cleaning up after resources. I'd expect users of the API trying to clean up after themselves, and unregister when they don't need to know about finalization anymore.

Btw, since this issue is about restricting values for the token, should we prevent it from being the same object as target? While it may be obvious that using the target as holding value will prevent collection, it's not so much the case for the registration token. And I could really see the user trying to do that: "hey I don't care about this object anymore, let me unregister it"

[littledan]

I've updated the specification text in #73 to treat undefined as the absence of an unregistration token. Out of a sense of conservatism, a TypeError is thrown for other non-primitives. There are various ideas for what the semantics of those could be, if we do want to permit them as unregistration tokens:

• null could be treated like undefined, as the lack of an unregistration token
• Other primitives (including null?) could be treated as an actual token, which could be more convenient than creating an object around them

I think we've heard high-level arguments for why these make sense, and it'd be great to add more concrete use cases, e.g.,

• Cases when you want to use an unregistration token and it's not convenient for that to be an object
• Cases when null may flow in representing a missing unregistration token (as opposed to just calling register with fewer arguments)

[zenparsing]

For the curious, here's what the API might look like with a cancellation-token-like design:

let abortController = new AbortController();
finalizationGroup.add(obj1, holdings1, abortController.signal);
finalizationGroup.add(obj2, holdings2, abortController.signal);
abortController.abort(); // Unregister both

[littledan]

@zenparsing The cancellation token design seems like a good idea to me, but ultimately, this is a fairly superficial difference. One could be implemented in terms of the other in just a few lines of code.

I'd suggest we consider adopting that design if cancellation tokens are ready for it in TC39 (or if the committee is OK with WHATWG-dependent layering), when this proposal would otherwise go to Stage 3, but that we shouldn't wait on it if TC39 has no particular cancel token to reference.

[erights]

I like the cancellation token idea. Aside from the WHATWG demands, we have had a cancellation token design ready to go for a long time. Someone should try re-raising it in tc39 to see if the committee still feels constrained by strange demands from WHATWG.

Attn @domenic

[littledan]

@erights Do you have any thoughts on what we should do for strings and numbers, if someone tries to use them as an unregistration token?

[erights]

Do you have any thoughts on what we should do for strings and numbers, if someone tries to use them as an unregistration token?

I do not yet have a strong or confident opinion. However, my inclination is that unregistration tokens should only be things also accepted as weakmap keys, i.e., objects.

I do not have any strong arguments to offer, either pro or con. I would be interested in hearing such arguments.

[gsathya]

Given that we don't see a use case for allowing non undefined primitives as tokens, should we throw for now? That seems the best path forward (in terms of web compat), if we want to revisit this later as we learn about more use cases once WeakRefs are used more often.

[erights]

Yes, please throw. They would then be given the same treatment as WeakMap keys. Thanks.

[gsathya]

Great, in that case we can close this issue as the current spec already has this behavior.

[devsnek]

I had a WeakValueMap implementation that used the key in the map (which is usually a primitive value) as the unregistration token. This was a surprising change to see.

[littledan]

@devsnek Do you have a more detailed reference you could provide? Is this open source?

[devsnek]

@littledan sure: https://gist.github.com/devsnek/e3ee8be1fc235e2bee43b2c1cd262adf

[mhofman]

@devsnek, you might be able to work around it by using the weakref object as the unregister token (which should not be held strongly) but I agree it's not optimal.

I believe I would still be able to implement my shim with primitive values as unregister token, but it would definitely make it more complex now that I leverage a weakmap to not hold the token strongly

[mhofman]

Here is what it would look like: https://gist.github.com/mhofman/fb6497e29e1eaaf522363c6d2d8217c6

Also fixed a slight inconsistency in delete.

PS: sorry for any typo, edited on a phone

[devsnek]

that does indeed seem to be a workable approach 👍