dom

dom copied to clipboard

I am not quite sure when this happened, but it seems like fairly recently... the change to add reason to AbortSignal was done in a somewhat breaking way, and I'm just finding out about it given that this breakage propagated to one of my libraries that uses/extends AbortController and AbortSignal.

In particular, my concerns are:

1. The definition of reason as READ-ONLY breaks libraries (like CAF) that had previously been monkey-patching/polyfilling a reason property onto AbortSignal instances. More about the breakage can be read here: https://github.com/getify/CAF/issues/28

2. A BREAKING CHANGE was also made to the abort(..) method. The change is that prior, if a abort() call was made with no argument, nothing adverse happens. But now, if you call abort() with no arguments, the reason doesn't stay set as undefined, as one would expect from PRIOR behaviors... rather, it gets set to a non-falsy DOMException error. That is, in particular, rather surprising, given that abort(undefined) -- explicitly passing in undefined -- avoids the DOMException value for reason.

Both of these changes may indeed have valid rationale, and I would certainly appreciate seeing where such discussions happened (as my searches thus far haven't turned up anything).

However, they conspire together to create a breaking change that crashes my CAF library. CAF has been around for 4+ years, is downloaded over 5,000 times per week on npm, and has over 1,200 stars on GitHub. As such, there's clearly a decent amount of sites/apps out there using CAF, and all of them are now broken as a result of these two changes.

I've done a pre-release of CAF to try to fix this crashing in a backwards-compatible way -- will do a full release once affected users report that it addressed the breakage -- but I have no idea if, or how quickly, all those sites/apps who are using CAF will update to my newest library release.

I would like to ask if it's possible to unwind change, even if temporarily, so browsers (like Chrome) are not breaking CAF v14.0.0 and prior?

For a context: The PR where reason was introduced https://github.com/whatwg/dom/pull/1027 Corresponding Chromium issue https://bugs.chromium.org/p/chromium/issues/detail?id=1263410

It's unfortunate this has happened, but those are not considered breaking changes. I thought it was somewhat widely known that patching objects can lead to issues in the JavaScript community?

Also, abort() and abort(undefined) behave identically. And I've no idea why one would expect anything about reason "PRIOR" as that was not a member of this object.

I appreciate that this doesn't meet the official designation of a breaking change. And I'm not trying to argue it is.

I still consider it "breaking" in an ergonomic sense (and practical sense, since it literally has broken libraries/sites).

When new properties get added to already existing APIs (objects), it has been my observation that they are not usually "locked down" in the sense of being defined read-only, as doing so limits user-land (libraries or apps). If there's a reason to protect a property, like security/privacy/etc, sure. But native APIs have capabilities that user-land code does not, like storing state in internal slots. So allowing developers to set (or change) the value of a public property, such as reason, even if that didn't affect any of the behaviors internally, seems like it would have been a more permissive/open sort of change to an existing API.

If you had known there are libraries that were already setting reason, prior to the spec change, would you have been more likely to leave it as a normal writable property to avoid as much potential breakage as possible?

Note: CAF is not the only (potentially) affected library. There are other small "polyfill" type utilities that defined/extended AbortController / AbortSignal.

With respect to the "PRIOR" behavior of abort() with no argument, here's where I'm coming from and why it feels breaking, ergonomically, to me.

Say I'm a site author and I observe that whether I pass an argument to abort() or not, it doesn't change any behavior. It doesn't set a reason. But because of how JS works, if I access a reason property on the signal, I just get undefined. So it feels like the lack of providing a reason results in an undefined value for a reason property.

So I write a very simple function like this for my own app:

function doAbort(cont,reason) {
   if (!cont.signal.aborted) {
      cont.signal.reason = reason;
      cont.abort();
   } 
}

In some places, I pass a reason, and in many others I don't. In some places, I actually intentionally pass an explicit undefined. Elsewhere, I can look at a signal instance and do checks like this:

if (!signal.reason) {
   console.log("unknown reason");
}

I end up doing these sorts of simple checks in dozens of places throughout my app.

I have effectively treated this abort() / signal mechanism as a simple information channel for my app to make flow control decisions. And it seems to work just fine.

When I later find out about reason being officially added to the spec, I'm excited and happy. I figure I can change that one function (simplify it) and skip setting reason (and pass it to abort(..)), and otherwise my information channel should work correctly.

I try that, but I'm dismayed that my whole site still seems broken. Eventually I realize it's because not only is there now a reason property, but even in places where I intentionally/explicitly pass it in as undefined, the reason property is not the falsy undefined value my intuition expected. It's a truthy object!

And when I look at the object, it's a DOMException object! Whoa, wait. So are they telling me that reason is essentially a required argument, because if I don't pass it, or even pass it as undefined, I see an exception!?

But wait, the docs (and spec!) don't really call this a "required" argument. MDN indicates it's optional. So I'm confused.

Ergonomically, if a thing has not existed before, and if it now exists but is treated as "optional", generally you just expect in places where you don't use it for it to behave as not used. In JS land, that USUALLY looks like a property being /remaining undefined.

This DOMException approach contradicts that intuition/expectation, because semantically it signals that reason is required. And I have to change every place in my code that either sends, or inspects, reason, to account for that new "requirement".

The DOMException doesn't throw, which would be even more hostile. But it does show up in my checks of reason, so it nonetheless feels like an exception telling me I omitted a required input.

It sure feels like a "breaking" change that now reason is effectively/semantically required and can't just be left/set undefined.

And I have a lot more code to change to adhere to this "breaking change".

---

In reality, the change in CAF to avoid setting the read-only reason property, only for newer envs -- but keeps setting it for older envs, as CAF has done for 4 years -- was like 5 or 6 lines of code. Not a big deal. I was relatively happy.

But then I realized the DOMException part... and that turned out to be much more disruptive to CAF's code. To preserve CAF's prior existing behavior (and thus the behavior sites/apps rely on), it took me 5 hours of effort yesterday and almost 200 lines of code added/changed throughout the library.

I hope maybe that perspective helps explain why this feels breaking even if it's not officially labeled as such.

We add many readonly attributes to objects all the time. The only place where we necessarily have to be somewhat cautious is the global object and certain EventTarget objects.

I strongly recommend not trying to guess how the web platform might extend objects going forward as indeed that can result in a lot of work.

I would typically never touch/extend built-in objects.

Cancelation has been such a HUGE gap in JS (and the web platform) for decades that I felt CAF had a responsibility to fix these gaps 4 years ago, mostly in response to AbortController finally being added.

But I was deeply dismayed that it wasn't added with something as obvious/helpful as a reason-tracking mechanism. I fished around for why, and I didn't find anything concrete.

Reluctantly, I decided CAF couldn't wait around in case a future change would address that gap. As it turned out, waiting 4 long years would have been, IMO, an unacceptable delay in addressing what people needed.

It was important to me to embrace the ecosystem, and (in particular) interop with things like fetch(..). That unfortunately meant I couldn't just wrap around AbortSignal (as fetch(..) wouldn't recognize them). Also, I can't even sub-class AbortSignal because I don't control the creation (done inside of AbortController automatically). I don't see any other way I could have balanced all this other than to decorate AbortSignal instances. I didn't want to, because I didn't want to face a day like yesterday in the future. But it was a risk I felt I needed to take.

I had thoughts about what it would be like if reason was later added. I intentionally designed CAF to work seamlessly with such a change, or so I thought.

I did not anticipate the read-only bit, but again that turned out to be an easy'ish accommodation to work around. I honestly never would have even remotely dreamed of the DOMException-on-omission bit. That part in particular just feels so unlike how features are typically added to existing APIs, as it feels to me like deliberately "breaking".

Lessons learned, for sure. But honestly, the 4 year gap here in some ways validates my reluctant decisions back then.

Again, if there are strong motivations for read-only and DOMException-on-omission, they are not at all obvious to me thus far. I would appreciate links to any further historical context onnsuch decisions.

Before the change there was a singular reason, exposed in APIs that ended up rejecting a promise (i.e., an "AbortError" DOMException). So that's what abort() continues to do. Passing another reason will reject those promises with the given reason. You can find discussion and motivation in the PR linked above.

I don't think there's anything left to do here.