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 hang. Eventually found, that 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;
};

Code Snippet 1:
Old version

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

Code Snippet 2:
New version

source: https://github.com/archiverjs/node-archiver/blob/dd7f10d/lib/core.js#L759-L792

Bad interface changes

The most painful thing about the new code is not that it has the same name while returning something new, this can be forgiven, since the upgrade was not minor but major upgrade. Fair is fair in major upgrade methods can change and retain the same name, might not be the best, but can happen.

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. Most issues have already been solved by someone previously.

Originally I've posted this on https://szab.it/posts/bad-dependencies-and-bad-interface-changes/