Bad dependencies and bad interface changes

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

Bad interface changes

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);
    });
  });
}

Bad dependencies

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:

  • we have linux
  • we have the files/folders accessible
  • we could have the 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 millions
  • offload processing from the main app, so it can still serve other requests without an issue
  • use built-in NodeJS module that is not likely to change interface and also tested by more than a lonely lib from npmjs.org

Conclusion

It 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.