Consider using AbortController rather than a stop() method

[reillyeon]

Somehow #147 became about cancelPush() rather than cancelWatch() (now stop()). push() now takes an AbortSignal but NFCReader still uses the start() and stop() pattern.

[kenchris]

Yes, which was due to consistency with Generic Sensors.

It is probably also a much more long running task than a push. I would like to hear pros and cons for changing this

@anssiko @tomayac @domenic @slightlyoff

[tomayac]

Consistency definitely is a big plus, Wake Lock recently also switched over to AbortController.

[reillyeon]

Sketchy example,

const reader = new NFCReader();
reader.onreading = e => {

};
const controller = new AbortController();
reader.start({ signal: controller.signal });

// Later...
controller.abort();

Pros:

• Consistency.
• Ability to cancel multiple operations with a single call.

Cons:

• Verbosity.

[kenchris]

I think we should just do both. Then you get consistency with newer specs and generic sensors, and you can remove the "cons: verbosity"

[tomayac]

One thing about AbortController that on a conceptional level bothers me is aborts leading to AbortErrors (i.e., a "bad" thing) that you should catch. In most cases like here, aborts are not "bad", you just stop something, as you're done. Example from Wake Lock:

WakeLock.request(type, {signal})    
.catch(err => {
  console.error(err.name, err.message);
});

[domenic]

The promise should settle, one way or another. If fulfillment implies you got the wake lock, then you should definitely reject if you cancel and don't get the wake lock.

[tomayac]

I’m more concerned about “planned aborts”, check this contrived example (see console). When you click the window, the Wake Lock is released. All planned, yet an “error”.

[domenic]

In that case, how do you suggest the promise settles?

[tomayac]

Coming back to the initial question of the present issue, I think a stop()—or release() or cancel() or whatever makes sense in the concrete context—method would have been cleanest, but I guess this discussion dates back to whatwg/fetch#27 and also https://github.com/tc39/proposal-cancelable-promises, and in the end AbortController was what was agreed on.

As @jakearchibald wrote on Web Fundamentals: "You don't often want to show an error message if the user aborted the operation, as it isn't an "error" if you successfully do that the user asked. To avoid this, use an if-statement […] to handle abort errors specifically."

Anyway, +1 for consistency and AbortController.

[beaufortfrancois]

I would strongly advocate for the use of the primitive AbortController. Reusing the same primitive everywhere we can has multiplicative beneficial effects throughout the web platform.
And I'd replace the start/stop pattern with a simple read(or scan) method.

const reader = new NFCReader();
reader.onreading = e => { /* ... */ };
reader.onerror = e => { /* ... */ };

const controller = new AbortController();
controller.signal.onabort = e => {
  // We're done waiting for NFC tags.
});

reader.read({ signal: controller.signal });

// Stop reading NFC tags after 3s.
setTimeout(() => controller.abort(), 3000);

[zolkis]

Maybe nitpicking, but reader.read() would give the false idea the script starts an NFC read.

What is useful from NFC point of view is setting up the read and error handlers.
In addition, we set up the AbortController. The name of that setup method should just reflect the fact we are setting up an AbortController. Please suggest another name there :).

On the other hand, I don't see that aborting a read would be a real use case: the user needs to hold the device close to an NFC device, and in the same time cancel the read, or do something else that results in losing focus/visibility that would suspend the read? and script would like to override that with rather canceling than suspending? Sounds a bit fuzzy to me.

IIRC the start() and stop() methods were added because Sensors compatibility and because setting up event handlers cannot have side effects. But IMHO implementations would be able to handle NFC reads by just having (or not having) event handlers set up for reads with given optional filter. No start() and stop() methods are actually needed IMHO.

But if you have a use case for explicitly aborting an NFC read (from a script), instead of just pulling away the device from the NFC device by the end user, please speak up :).

Unless we have a compelling use case, could we try shipping without abort support for now and add it later when having use cases?

[kenchris]

I am fine with the idea. Makes sense especially for SPAs etc where you change view.

We already use 'push' (single future write) for NFCWriter, so using 'scan' (multiple future reads) seems fine

[zolkis]

Using the name scan() with an AbortController sounds good to me.

I would still like to get clarity on what use cases we might have for aborting NFC reads (or scans). Ideas still welcome there.

[kenchris]

OK, so to close this off and with my active editor hat on, I am making an executive decision that we move ahead with the rename to "scan" and integration with AbortController

Some of my reasoning.

scan is a fine name and it aligns better with push, like we don't have NFCWriter.write as it doesn't write immediately (hense push), and so NFCReader.read would be confusing as it is not reading immediately and can do multiple reads... hense scan/push fits well with the nature of NFC

AbortController is already being used in the NFCWriter.push, so it is quite consistent with the spec itself, as well as with multiple other modern specs. This is the new way we do things, whether you are a fan of AbortController or not.

You could argue that that is not used by Generic Sensors, but I would argue that it would have been if they were designed today, and still might be for the GeolocationSensor.

For me personally, it also seem quite nice to be able to use one signal to cancel multiple things in a single-page application when switching views, without requiring having access to all kind of internal state of the views that are now being hidden.