Hi friends, I have been attempting to use the promise that gets returned from the finalize method. In this implementation I have noted that the implementation seems to have a couple flaws. Maybe I am missing something here?
Here is the method in question:
|
Archiver.prototype.finalize = function() { |
Archiver.prototype.finalize = function() {
if (this._state.aborted) {
this.emit('error', new ArchiverError('ABORTED'));
return this;
}
if (this._state.finalize) {
this.emit('error', new ArchiverError('FINALIZING'));
return this;
}
this._state.finalize = true;
if (this._pending === 0 && this._queue.idle() && this._statQueue.idle()) {
this._finalize();
}
var self = this;
return new Promise(function(resolve, reject) {
var errored;
self._module.on('end', function() {
if (!errored) {
resolve();
}
})
self._module.on('error', function(err) {
errored = true;
reject(err);
})
})
};
When reviewing I noted a couple things that I was hoping to get fixed. First, the method, when the state is aborted or finalized, doesn't return a promise and, hence, if consuming the promise, one goes off into the ether. I was thinking that, maybe, the original implementer was thinking that since the emit occurs in the code block there for the abort, that the reject would get picked up later on in the promise but, as the method returns "this", no such reject would be caught.
Second, it looks like the _finalize call should be made after the events are wired in on the following lines, in the promise block.
Now, I have code to fix all of this but, due to promise return and this return in the same method, I thought that maybe I was missing something here and that a straight PR might not be well considered?
In any case, here is the code, I would love to hear if there is a reason why I shouldn't just PR this in?
Archiver.prototype.finalize = function() {
const self = this;
return new Promise(function(resolve, reject) {
let errored;
if (self._state.aborted) {
this.emit('error', new ArchiverError('ABORTED'));
return reject("The operation was aborted.");
}
if (self._state.finalize) {
this.emit('error', new ArchiverError('FINALIZING'));
return reject("The archiver is currently finalizing.");
}
self._state.finalize = true;
self._module.on('end', function() {
if (!errored) {
return resolve();
}
});
self._module.on('error', function(err) {
errored = true;
return reject(err);
});
if (self._pending === 0 && self._queue.idle() && self._statQueue.idle()) {
self._finalize();
}
});
};
Hi friends, I have been attempting to use the promise that gets returned from the finalize method. In this implementation I have noted that the implementation seems to have a couple flaws. Maybe I am missing something here?
Here is the method in question:
node-archiver/lib/core.js
Line 759 in 34cd7b5
When reviewing I noted a couple things that I was hoping to get fixed. First, the method, when the state is aborted or finalized, doesn't return a promise and, hence, if consuming the promise, one goes off into the ether. I was thinking that, maybe, the original implementer was thinking that since the emit occurs in the code block there for the abort, that the reject would get picked up later on in the promise but, as the method returns "this", no such reject would be caught.
Second, it looks like the _finalize call should be made after the events are wired in on the following lines, in the promise block.
Now, I have code to fix all of this but, due to promise return and this return in the same method, I thought that maybe I was missing something here and that a straight PR might not be well considered?
In any case, here is the code, I would love to hear if there is a reason why I shouldn't just PR this in?