Recently one of my colleagues has shared his pain about a nodeJS package that we use to zip files.
His issue was that the library stopped generating the zip files and just hangs. Eventually found, that in the project the package in question has been upgraded, but with a small issue. The newer version has changed the interface and the same method instead of returning the instance it returns a Promise.
Archiver.prototype.finalize = function() {
if (this._state.aborted) {
this.emit('error', new Error('finalize: archive was aborted'));
return this;
}
if (this._state.finalize) {
this.emit('error', new Error('finalize: archive already finalizing'));
return this;
}
this._state.finalize = true;
if (this._pending === 0 && this._queue.idle() && this._statQueue.idle()) {
this._finalize();
}
return this;
};
source: https://github.com/archiverjs/node-archiver/blob/0.15.1/lib/core.js#L489-L507
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);
})
})
};
source: https://github.com/archiverjs/node-archiver/blob/dd7f10d/lib/core.js#L759-L792
The painful thing about the new code isn’t the fact that it has the same name and returns something new, since the upgrade was not minor but major upgrade.
The thing that actually bad in my point of view, that it can return two
completely different things. Till there is an if that is being triggered it
returns the instance of the class, but once those are exhausted it returns a
Promise.
Icing on the cake that in this case the instance that it returns is a stream,
that we were just piping, but now at some point it will turn in to a Promise.
Generally it is a good rule of thumb that a method should always return with the same type, no matter on the logic inside. (Unless your method is a type selector or something like that :D )
For the above code I could imagine that the original finalize could have
remained the same, always returning a stream. There could have been a new method
created that would give back a promise and also retaining backward
compatibility. E.g.: introducing an asPromised method.
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();
}
return this;
};
Archiver.prototype.asPromised = function() {
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);
});
});
}
Now over for the necessity of this npm package.
This package is used in NodeJS, so we don’t send it to do magic on the front-end, but we use it in a controlled environment: a NodeJS app running in a Docker container wrapped in a Kubernetes pod.
For the docker container we use GNU/Linux, so we can have other programs in there as well, that can be found for Linux.
The files that we zip are on an NFS drive, so we don’t need to stream it, we know the location of the file as well.
Here comes the idea:
zip program on the system So in effect we could axe a third party npm package and use child_process.exec
or child_process.spawn method’s from NodeJS itself. The extra 3rd party in this
case would be the zip program, but think it is safe to say that command is a
battle tested, fire hardened program. Which is not likely to cause issues or
change interface. For me an extra benefit of this approach, that we can offload
processing from the main app to a separate CPU thread aaaand we can also use
GNU’s nice utility to set the importance of the task. Like this we can also
ensure that the main app will always have enough CPU time to respond and the zip
command won’t eat it up from the app as well.
Gains with this:
zip is a reliable program that has and is being tested by millionsIt is good to think through as what is the goal and not just pick an npm package off the shelf. The issue you’re facing has most likely already been solved.