Implement support for optional LIFO ordering

[ggoodman]

No description provided.

[ggoodman]

I'd love your thoughts on whether this might be a breaking change in semantics whereby all callbacks are run in the same future tick vs. each being possibly scheduled in different future ticks.

[dougwilson]

Hi @ggoodman I would suggest publishing as a different module 👍

[ggoodman]

@dougwilson thanks for taking a look, but it would have been great to hear some of your reasoning behind your decision.

I thought the community would benefit more from this feature being in the on-finished module than if I had published a fork whose code would undoubtedly diverge over time. Having seen two other users make the same request, and an on-finished-lifo module in npm (whose codebase has diverged) I thought you would be more willing to explore the feature.

I'm disappointed to have spent my morning working in an unfamiliar coding style and codebase to have the PR summarily dismissed like this. As a new contributor, this was a very frustrating experience for me. If the maintainers are not open to new features or behaviour, it would be helpful to have the project make this explicit to potential contributors.

[dougwilson]

Hi @ggoodman I'm sorry you spent your time -- you could have always asked before starting work to at least get a gauge, or infer based on the issues you commented in.

[dougwilson]

Ultimately you should use res.on('finished', cb) in Node.js directly -- this module is pretty much just supposed to be a fill-in for that functionality, from a time when Node.js ever supported it (hence the name on-finished -- a play on what the EE signature would look like). If Node.js adds the ability to specify LIFO / FIFO in their event listeners (like res.on('finished', cb, true)) then it could be added here, but the scope of this module is a drop in.

From #18 (comment) :

This module will only run in the same order as even listeners, mainly because we are attaching event listeners. [...] Ultimately, the order is only that of event listeners.

From #17 (comment) :

The ordering is unfortunately not a negotiable part [...] that the ordering matches that of event listeners, which we did not define and does not provide any real interface to run in reverse order.

I don't like to copy-and-paste previous comments because it feels rude, but you're not asking any questions to clarify already-existing statements. You commented on those issues, so I had assumed you read through them, but perhaps that was a bad assumption, and I'm sorry I closed it assuming you did.

I hope this clarifies the decision.

It is always a risk to blindly open a pull request to a repository, especially if it is going to take some time and there isn't an issue already open asking for someone to implement it. I would suggest opening an issue with an idea or question if you think that if the pull request is not accepted it would be a waste of your time, as that would be a way to hedge against that.

[ggoodman]

Hi @dougwilson, I had read through those issues. I probably read too much between the lines thinking that the reason this had not been implemented was because of time and complexity:

for the one part it being a nightmare if multiple versions are used at once with different semantics

Ultimately, my PR doesn't seek to add true LIFO but allows callbacks to be pushed to the front of the callback queue instead of to the back. The bulk of the code is to support these new semantics in the scenario where the underlying object has already been 'finished' and to provide thorough tests.

I thought that I had considered the scenarios in such a way that this would be a non-breaking, but useful feature addition. I will take an alternative approach that simply layers the behaviour I want on top of on-finished. Perhaps it was a mistake not to have done this from the beginning.

It is always a risk to blindly open a pull request to a repository

Indeed. It is difficult to balance the desire to submit a high-quality PR vs asking for something. I hope to do more of the former than the latter.

[dougwilson]

Indeed. It is difficult to balance the desire to submit a high-quality PR vs asking for something. I hope to do more of the former than the latter.

Right, I get it, but wouldn't that still be accomplished by opening an issue with "I want to make a PR that ..." or similar? In other words, the issue would be clear that you're asking in order to make a PR, not just generically asking for someone else to make the change. I'm not sure how that could be construed in that you would be asking for something vs making something if it was clear your intentions in an issue.

I did take a quick look over the PR, and though there were a couple of questionable changes, the issue wasn't if it was breaking or not -- it is about the scope of the module. This module should simply fade away into the past and no one should use it; why should a module be necessary at all just to do something everyone needs? Node.js HTTP should emit a finished event on req or res and be done with it so you don't have to reach into user land for a trivial task. This module is complex because it's trying to implement something at a distance that should just be in Node.js core code directly.