An interesting conversation happened in @Clemdz's pull request (here) that I thought I would mention here, along with some thoughts about it.
@Clemdz attempted to do a little refactoring on the code that was supposed to model his ideas, so that it better reflected what he was proposing. In the process of this refactor, he ran into this function (I'll post the version using the "excepts" syntax I had originally proposed in this post, as it's more concise)
function givePermissionToUsers(permission, userIds, { userIdDoingRequest }) {
userManager.assertUserHasPermission(userIdDoingRequest, PERMISSIONS.view) excepts 'Unauthorized'
userManager.assertUserHasPermission(userIdDoingRequest, PERMISSIONS.update) excepts 'Unauthorized'
const users = []
for (const userId of userIds) {
users.push(
userManager.getUser(userId, { userIdDoingRequest }) excepts 'NotFound'
)
}
for (const user of users) {
if (!user.permissions.includes(permission)) {
const newPermissions = [...user.permissions, permission]
userManager.setProperty(user.id, 'permissions', newPermissions, { userIdDoingRequest })
}
}
}
@Clemdz wasn't sure why there were two for loops, even after looking over the existing variations of this chunk of code, and decided to refactor it into one for loop.
Which was a breaking change.
In my original post (see here), I had mentioned that the problem I hoped to solve in this thread was that of trying to make it obvious when reordering certain statements would cause a breaking change. This function that was wrongly refactored was supposed to be a demonstration point of this type of breaking change I was hoping to avoid.
In the first for loop, we're retrieving all of the users using userManager.getUser(). This getUser() function could potentially throw a 'NotFound' exception if a particular user does not exist. If none of the getUser() function calls fail, then we proceed to update the permissions property on all of the users. By consolidating this into one for loop, we are now causing updates to start happening, before all of the NotFound checks were done. In a single-loop approach, if a getUser() call fails half-way through, then half of the users will end up with updated permissions and the other half will not.
When an exception is thrown, it should leave the system in a good state, so that the caller can handle the exception and recover from the issue (if it's not left in a good state, then a fatal error should be thrown instead) - I wouldn't really call "half of the users updated" a good state that can be recovered from (or, at the very least, it's not an ideal state to leave it in, but I'm sure there are some scenarios where this behavior isn't all that bad).
I take this event as living proof that none of the proposed explicit-exception systems really do a great job at solving the refactoring-issue I was hoping to find a solution for in this thread. Certainly, they help - because the exceptions are explicit, a reader of the code can see that certain exceptions were being thrown in the first for loop, and that this refactoring is indeed a breaking change (Without the exception being explicit, it would require the reader to follow the different function calls to see what kinds of exceptions different functions may throw) - but just because it's possible to figure out that the refactoring would be a breaking change, does not mean it's obvious or easily deducible. You almost have to be specifically looking for this type of thing in order to notice it.
@Clemdz mentioned that simply adding some comments explaining the odd ordering of the code would have gone a long way. I would add that unit tests can help catch these types of accidental breaking changes too. This series of events has caused me to think long and hard about this class of bugs, and I realized that I've been looking at this issue from the wrong angle the whole time. These "accidental refactoring" issues I'm trying to prevent are all related to trying to update a resource in an atomic way. In the code snippet presented above, I'm hoping to fetch all user information before updating it, in order to ensure I can perform the updates as a single atomic action - either all of the updates happen, or none of them do. Moving exception-throwing logic first is just one way to handle atomic updates, and only works in certain scenarios. Other techniques are sometimes needed, like rolling back an update if an exception does occur, etc. I'm now doing some additional research into this topic to see what types of libraries/patterns exist to help make atomic updates easier to work with and less error-prone.
The point I'm getting at here is that the original class of bugs I was hoping to solve in this thread is better solved through the means of atomic update libraries/patterns than through an explicit-exception system (though explicit exceptions certainly help out). I'm sure a lot of contributors to this thread are here mainly with the hopes of getting a better exception system in Javascript anyways (and probably didn't care as much about this atomic-bug thing as much as I did).
The tl;dr
I think we can change the direction of this thread to have the focus of enhancing Javascript's exception system in general, and forget about atomic-related bugs I was originally trying to address in my original post.