Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Property name "signal" carries no meaning #147

Closed
mk-pmb opened this issue Jun 6, 2024 · 11 comments
Closed

Property name "signal" carries no meaning #147

mk-pmb opened this issue Jun 6, 2024 · 11 comments

Comments

@mk-pmb
Copy link

mk-pmb commented Jun 6, 2024

A property should only be named "signal" in contexts where there is really only one thing imaginable that could be signaled, and this is not such a case. (E.g. a subscriber might want to provide a signal for when the first event was successfully handled, e.g. for monitoring in cases where the first event handling requires substantial effort like breaking the seal on a new output tape.)

My first spontaneous idea for a better name would be noLongerInterested. It might not be the best name either, but a step towards a better name.

@Jamesernator
Copy link
Contributor

The naming of signal is mandated for Web APIs by the AbortSignal spec.

@domfarolino
Copy link
Collaborator

Well, sort of. It is mandatory for AbortSignal-consuming APIs to consume the signal in a dictionary, with the dictionary member name signal. But I don't think it is mandatory to expose it on an object like Subscriber with the interface member name signal.

A property should only be named "signal" in contexts where there is really only one thing imaginable that could be signaled

Still, I am unconvinced by this. The signal is there for the consumer (of the Observable) to express that they are no longer interested in receiving the values that the Observable is producing. Whatever their reason, the Observable-side result of "shutting down" will be the same. Whatever their reason, this is meant to be communicated through AbortController#abort() as the abort reason. That's the place for the intention of the abort to be communicated.

@mk-pmb
Copy link
Author

mk-pmb commented Jun 7, 2024

That's the place for the intention of the abort to be communicated.

Agreed. My point was about other kinds of signals, that are not about aborting the subscription. Even if we were omniscient enough to 100% know that the Observable will never have any other kind of signal, it may still trip up readers of code that uses the Observable. Idealistic programmers may feel a need to add a clarifying comment anywhere they use the signal property. Other programmers will omit that comment and someone fixing that part of the code years later will think this line is about the other signal they intended to debug. Thousands of life hours wasted on clarification and having to remember.

@domfarolino
Copy link
Collaborator

My point was about other kinds of signals, that are not about aborting the subscription.

What other kind of signals are there? The primitive is literally called AbortSignal after all. You use it when you intend to.... abort something. Like a fetch, or an event listener, or an Observable, etc.

@mk-pmb
Copy link
Author

mk-pmb commented Jun 10, 2024

I think our misunderstanding perfectly illustrates the argument. From the Readme:

let controller = new AbortController();
observable.subscribe({
  next: (data) => {
    if (data > 100) controller.abort();
  }}, {signal: controller.signal},
});

The word signal appears twice in that code. I'm rather sure that only the 2nd is mandated by the AbortController spec, and there, it makes sense. The first one however, is the one I meant should carry more meaning.
A first step towards clarity at least in this discussion may be {unsubscribeWhen: controller.signal}, and I hope we can come up with even better names.

@bakkot
Copy link
Contributor

bakkot commented Jun 10, 2024

The AbortController spec says that APIs must

Accept AbortSignal objects through a signal dictionary member.

So no, in fact both occurrences of signal in that code are mandated by the spec.

@mk-pmb
Copy link
Author

mk-pmb commented Jun 10, 2024

Wow, that seems to be a surprisingly hostile spec. Still, we can work around it by containing the madness in something that clarifies the meaning, e.g.:

observable.subscribe({
  next: (data) => {
    if (data > 100) controller.abort();
  },
  unsubscribe: { signal: controller.signal },
});

Edit: With that, people could even choose to pass the entire controller instead of that signal-property-extraction object. In the API docs, we can assure them that from that object we won't keep around references to anything besides the value of the signal property. With that assurance, they can write concise code without worrying about impeding on garbage collection.

@domfarolino
Copy link
Collaborator

I just don't think this needs any clarification. AbortSignals are there to abort. For event listeners, that means unregister the event listener. For fetch(), that means stop fetching. For Observables, that means stop producing values & teardown. I am not convinced that there is any more work to do here, and since the current spec is exactly inline what DOM requirements and how other APIs use AbortSignals (i.e., by consuming AbortSignal through a signal dictionary member, and not consuming "the whole AbortController" which feels very wrong), I think we can close this.

@mk-pmb
Copy link
Author

mk-pmb commented Jun 10, 2024

AbortSignals are there to abort.

Yes. My problem was, and still is, that from our side of the API there is nothing that shows that we expect an AbortSignal. Just some generic signal that the reader has to guess what it may be meant for.

@mk-pmb
Copy link
Author

mk-pmb commented Jun 10, 2024

Also, fortunately, there was a line missing from the above the quote:

Any web platform API using promises to represent operations that can be aborted must adhere to the following:
• Accept AbortSignal objects through a signal dictionary member.
DOM spec, chapter 3.3 "Using AbortController and AbortSignal objects in APIs"

Are we really "using promises to represent operations that can be aborted"?
The act of subscribing seems to be immediate/synchronous, so its result (undefined/throw) doesn't need to be a promise.
The resulting subscription itself is not an operation.

Edit: Below the algorithm box it says

APIs not using promises should still adhere to the above as much as possible.

but I still suggest we try and fix the upstream requirement rather than infecting our own spec with such a hostile-to-readers stance. The only positive argument I could find in the other thread is that other APIs use that property name already – which is a non-starter because their contexts may be entirely different.

@mk-pmb
Copy link
Author

mk-pmb commented Jun 11, 2024

We may even have an option that can work for both ideals: We can make an option unsubscribeSignal that is our default and will unsubscribe at any signal, and then for people who enjoy conformity over readability, we could additionally accept a generic signal property that will unsubscribe only if it is an AbortSignal.

I have no hopes of convincing you, I just want to bust all possible excuses for perpetuating the (potentially, if understood as some seem to have) reader-hostile upstream approach. (Still waiting for actual readability/ mistake-avoidance/ ergonomics arguments there.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants