.finalize(): Consider using the (err, data) callback style

[bf]

Hello,

during my use of node-archiver I have noticed that the .finalize() function just returns one argument written to the callback function. This kind of stands out in the rest of my code, so I wonder if there aren't any errors which could possibly be thrown during execution of this function. If there are errors which can be thrown then in how far can we return them to the callback function? This might be a place to apply the widely-used function (err, data) { .. } callback style.

If there are in fact no errors to return from this function, why is .finalize() asynchronous in the first place?

I'm just wondering and hope you can point me in the right direction.

[ctalkington]

finalize adds the extra closing bits to the file via the stream so it has to be async as it has to wait on the stream to close before it can give anything back.

errors are emitted as events so you can catch them with archive.on('error', function (err) { .. }).

[ctalkington]

also, i do plan to make these error events provide an object vs a simple string in the near future so that more detailed info can be provided.

[danmilon]

It's not about not being able to catch errors.

The problem with breaking the cb(err, res) convention is that finalize can't be used with control flow libraries / other code that expects the first argument to be the error.

[ctalkington]

@danmilon fair point. i think i can make this happen as the next major release since it has breaking changes. would really love to have a few testers for this project to help make it more robust in the long term.

[ctalkington]

you guys mention finalize but when I do it, id prob also do it for addFile as when i do the change i want it to be consistent.

[danmilon]

Definitely.

[ctalkington]

ok then. ill see about making the needed changes and releasing v0.3.0 in the next two weeks.

[ctalkington]

guys let me know what you think of the latest commits.

[danmilon]

Will do during the weekend.

[ctalkington]

@danmilon thanks. i've overhauled the docs and examples too so let me know what you think and thanks for your feedback thus far.

[danmilon]

So, as of 51a7dbf, finalize and addFile push the error in the callback, instead of emitting it.

My suggested flow is:

1. if callback present cb(err)
2. else .emit('error', err)

This way errors will never pass silently. If there is no error event listener, the node core will throw it, by design.
@ctalkington, what do you think?

[ctalkington]

hum I define a fallback callback that throws err so eitherway works. not sure my pref at this moment.

[danmilon]

ah, indeed. Currently you cannot use the library without callbacks because of addFiles having to be serialized. I plan to write a PR to fix this, in which case it does make sense to not provide a callback and centrally manage errors in the error listener.

If I send a PR with the changes I proposed earlier will it be accepted?

[bf]

Dan, I have already implemented a true asynchronous way to use addFile with zip archives. Please check out my fork https://github.com/bf/node-archiver where this is implemented. I havent sent a PR yet because I wanted to implement the same for creating .tar files.

The basic approach is to keep an internal queue which adds each file after the last one is finished. Because of this, I can use addFile with Step.js in a loop:

Step (
function () {
  var group_callback = this.group();
  _.each(arr, function (val, key) {
                                archive.addFile(source, {
                                    name: 'test'
                                }, group_callback());

  });
}
);

[bf]

Please see my Pull request #8 for further information. Thank you.

[ctalkington]

this callback style has been implemented in latest commits scheduled to be part of v0.3.0