Open LeaVerou opened 3 years ago
This question does come up sometimes, e.g., why not return this instead of undefined for append()
, delete()
, and set()
in https://url.spec.whatwg.org/#urlsearchparams (see https://github.com/whatwg/url/issues/90). My thinking on this is still roughly the same:
Another reason why I think the chaining pattern is bad is that it breaks command-query separation. Although the platform does not fully respect command-query separation, as you note with some exceptions, it's a good design principle to strive for in any API.
Certain functions, are primarily called for their side effects, and do not have an obvious return value.
However, it is common practice in the wild to make these functions return a value that is seen as potentially useful, so these functions can be used in the context of a larger expression, essentially allowing developers to combine what would have been multiple statements in one. For example, a common pattern is to return the function context, to allow for chaining (Fluent APIs). Another pattern is to return one of the arguments. The thinking is that if the developer does not need the return value, they can always not use it.
Some examples of such methods on the Web platform:
Map#set()
returns theMap
instance (and same withSet
and theirWeak*
versions)element.insertBefore()
returns the inserted node,parent.replaceChild()
returns the old child and so onarray.push()
returns the new length of the arrayObject.assign()
returns the first argumentperformance.mark()
andperformance.measure()
in User Timing L3 return the performance entry they create. Apparently this has been a conscious change to improve DX (source)element.dispatchEvent()
returnsevt.defaultPrevented
I'm not including examples of methods who are primarily called for their side effects, but return a value that communicates some aspect of what happened that cannot be obtained any other way (e.g.
media.play()
), because that’s not just a convenience.On the other hand, there are way, way more void methods that do not follow this pattern and just return
undefined
:element.setAttribute()
and friendselement.before()
,element.replaceWith()
etcDOMTokenList#add()
URLSearchParams#set()
element.insertAdjacent*()
methodselement.scroll*()
methodsadd/removeEventListener()
HTMLDialogElement#show()
What should be the TAG's position on this?
On one hand, since the examples where
undefined
is returned far outnumber those where a value is returned, it would seem that for consistency, we should encourage returningundefined
. On the other hand, developers have frequently lamented that these methods returnundefined
instead of something more useful, and there are entire libraries whose main (or even only) point is to wrap these functions and give them a useful return value. There doesn't seem to be any downside to returning a potentially useful value (since the function can still be called as a void), so it seems that returningundefined
is a wasted opportunity. However, the fact that the newer DOM manipulation methods went that route, which seems to have been a conscious decision since the legacy ones did return a value, makes me think there might be something at play here that I have not considered (perhaps @annevk or @domenic could provide some background here).If I really try to find downsides, here are a few I came up with:
childNode.before()
), there is no obvious correct choice, so picking one arbitrarily could make code harder to read. However, always returningthis
instead of an arbitrary argument doesn't suffer from any such ambiguity.undefined
encourages multiple shorter statements. However, when using fluent APIs in the wild, it's common practice to break the chained methods into multiple indented lines, which makes things easier to read than repeating the context on every line, which adds noise. (example below)Regardless, this seems like something worth discussing and resolving on so we can all be on the same page.
Footnote: Canvas API, current vs fluent
Compare: ```js ctx.clearRect(0, 0, 256, 192); ctx.save(); ctx.translate(96, 96); ctx.rotate(45*Math.PI/180); ctx.scale(1.2); ctx.drawImage(img1, 0,0,192,192, -288,-288,576,576); ctx.drawImage(img2, 0,0,192,192, -96 ,-96 ,192,192); ctx.drawImage(img3, 0,0,192,192, -32 ,-32 ,64 ,64); ctx.restore(); ``` with: ```js ctx.clearRect(0, 0, 256, 192) .save() .translate(96, 96) .rotate(45*Math.PI/180) .scale(1.2) .drawImage(img1, 0,0,192,192, -288,-288,576,576) .drawImage(img2, 0,0,192,192, -96 ,-96 ,192,192) .drawImage(img3, 0,0,192,192, -32 ,-32 ,64 ,64) .restore(); ```