Consider using the Promise API

[ijisol]

I'm glad to see this project starting to get updated again. I was completely convinced that it was not maintained anymore and published a fork package just 3 days ago, but I wish I could have known the future.

Would you consider using Promises internally? This does not change the API. It means using Promises as queues internally. I think it's much easier to understand, and it makes async support more sure.

I link the code I was rewriting.... https://github.com/ijisol/yazlite/blob/main/index.js

[thejoshwolfe]

Sorry for the unpredictability! The tides of my enthusiasm are just as mysterious to me as to the community, although I am getting better. I actually gave a talk about it here, if you're interested: https://www.youtube.com/watch?v=V-VO0Ua_b4g

Much easier to understand is definitely a plus, but i think that's pretty subjective. This line in particular jumps out to me as being confusing: https://github.com/ijisol/yazlite/blob/be23317e271243aa7ce9286044f9a6a859173c01/index.js#L464 , but that's probably due to me being unfamiliar with idiomatic promise paradigms in javascript. I think it's effectively a linked list iterator?

I'm also pretty sure this performance is worse: https://github.com/ijisol/yazlite/blob/be23317e271243aa7ce9286044f9a6a859173c01/index.js#L319 , due to opening the read stream too early. That would result in holding all the fds open, a problem addressed by #80 for the addReadStream() api.

I'm also unsure how throwing an error works in promises, like this one: https://github.com/ijisol/yazlite/blob/be23317e271243aa7ce9286044f9a6a859173c01/index.js#L315 , but then again, i'm not totally clear on the error handling semantics of master branch either. See also #72 . This is probably another case where I need to educate myself on modern javascript idioms.

I do like that using promising simplifies the entry.state management. I'm currently worried that calling pumpEntries() isn't happening at the right time in all cases, and the promise paradigm does help with that quite a bit. I'll revisit the control flow to see if there's something with the promise paradigm or a promise-like paradigm that could improve the understandability.

[ijisol]

@thejoshwolfe Thank you for the long and detailed answer.

The error handling is unclear because it is still being written. Hehe. Anyway, if the original project is being updated again, there is no reason to develop it further. The Promise API can handle errors through catch(), but I was thinking about how to handle errors when chaining multiple promises.

As for the performance issue of addReadStream(), it is because the code is based on v2.5.1. I think it can be fixed, but as I said above, I don't think there is any reason to do so.

What I wanted to show was the idea of ​​scheduling the sequential processing of the stream through then(). As you said, it can be called the "idiomatic promise paradigms" of JavaScript. The promise chained through then() is processed after all the previous promises are processed. I think it is clearer than pumpEntries() because it is guaranteed at the language level.

[ijisol]

I've been thinking about error handling for a while, and I admit it's best to continue using EventEmitter at async errors so as not to break existing APIs.

And, this is an example of changing Zipfile.prototype.end() to a Promise style API... https://github.com/ijisol/yazlite/blob/53f07bd0cfe98916ad2848791af6895803ca9b70/index.js#L421
I think it could be backwards compatible if it would be made into a separate method like endAsync().

[simonbuchan]

TLDR: I don't think this is super useful without essentially a full re-write.

Promises have a few pros and cons. The general jist of it is that they slightly decrease performance for the trade-off of being (often significantly) easier to reason about when used well. The biggest improvement though is the ability to use async / await syntax, which is much easier to read than equivalent callback code, particularly when control flow is involved.

For example, the existing code uses a pattern of a pumpEntries callback that needs to be called exactly whenever completing work on an entry that then pulls the next item off the list. A rough translation to a "properly" promise-based implementation would look like:

class ZipFile {
  entries = []

  // JS-native private members, if you haven't seen the syntax
  #entryPumpPromise = null

  addEntry(...) {
    const entry = new Entry(...)
    // added work for the entry pump, so start it if it's not running.
    this.#entryPumpPromise ??= this.#entryPump()
  }

  async #entryPump() {
    for (const entry of this.entries) {
      if (entry.done) continue; // wrote it on a previous call of #entryPump()
      try {
        // the magic, no need to thread pumpEntries through things that #writeEntry() may do.
        await this.#writeEntry(entry);
      } catch (error) {
        // You can catch any sync or async error with the same syntax, too:
        entry.error = error;
        this.#dispatchError(error);
      }
    }
    if (this.ended) {
      this.#writeEOCD();
      this.allDone = true;
    }
    // out of work, clear the promise so it can start anew.
    this.#entryPumpPromise = null
  }
}

I should also mention that more recent javascript implementations will correctly maintain the call-stack with correct async / await usage, though that's not a huge value for the likely usage here, where at best you would be awaiting .end(), so if it throws about invalid entry content you wouldn't get a stack automatically. A common workaround is to capture the stack in addEntry() with entry.stack = new Error().stack, then re-attaching that stack later when throwing.