Use MAX_LENGTH constant to check buffer size

[gpoole]

Replace hard-coded constant for max buffer size check with Buffer MAX_LENGTH when available.

[thejoshwolfe]

I actually prefer to use my own copy of the constant, which is documented explicitly in the API. If node later decides to increase the limit, I want my API to remain the same. The limit in yazl is based on node's limit, but it is now yazl's limit. If node were to reduce the limit, that would break my assumptions, but I don't think that's ever going to happen.

Thanks for the contribution though! And sorry for the delayed response.

[gpoole]

@thejoshwolfe sorry I should have been more explicit in the description: the motivation was that 64 bit versions of node were capable of creating buffers up to 4GB, so valid buffers are triggering this error and I assumed the limit you have here was related to the maximum buffer size in Node. The value of MAX_LENGTH is different on different architectures, so that's why I used it, rather than just changing the constant value here. Since I opened the PR, the limit has again been lifted to 8 PiB on 64 bit architectures in v23, which means buffers are effectively limited only by available memory now. Have a look at the API docs to confirm. If this check is indeed related to max buffer size then it might be worth reconsidering since the current limit is below what's possible in Node.

Rereading this some time later though I'm actually not totally clear on what the purpose of the check is? yazl can add files to a zip larger than 1GB via a stream, and it's not possible to allocate a Buffer larger than MAX_LENGTH as Node throws an error. Do you need the check at all?

[thejoshwolfe]

Huh. Now I'm also not totally clear on what the purpose of the check is. I don't like the idea of zlib compression throwing a runtime error when the compressed buffer would exceed the max size; that would be a pretty bad user experience. But limiting the input is only one of many ways to avoid that problem. (And the current limit doesn't even avoid that problem, I think?) It could also be solved by streaming the compression into the output instead of keeping the compressed buffer in ram. 🤔 (This also relates to #54 .)

It does seem compelling to allow the bigger buffers when they're available and to simply document that the size limit is imposed by node, and link to the API docs you gave. Those docs are pretty clear and include the change history and everything.

[gpoole]

Yes I think it makes sense that the principle behind doing the check is to try to avoid the output buffer getting too big by not allowing in huge inputs. Having thought about it, since it's checking an existing buffer that the user is providing, by definition it can't possibly be bigger than MAX_BUFFER, so checking length against that might actually be redundant anyway?

For now, might it be worth removing instead of modifying the check here since it sounds like it won't cause new problems and would allow bigger buffers to be added?

I'm not across the library enough to talk about the compressed buffer bit and I'm unclear on whether Node is going to throw automatically in that code, but it might already throw a RangeError that could be caught and wrapped with a friendlier message?