search

Conduitry / do-not-zip

Do not zip. Just store. // Mirror of https://git.chor.date/Conduitry/do-not-zip
https://conduitry.dev/do-not-zip
MIT License
25 stars 2 forks source link

Explicit Node dependencies imports #7

Open alexandercerutti opened 6 months ago

alexandercerutti commented 6 months ago

Hi @Conduitry, thanks for your work. I'm using for library on my project https://github.com/alexandercerutti/passkit-generator.

Someone in the issues was reporting me that do-not-zip seems to not be working on Cloudflare pages / workers, as that platform seems to have only a compatibility with Node.JS API. Also, they run ESM.

The "not working" seems to be only about the absence of explicit imports of Buffer.

I know that the last package update is dated over 5 year ago. So here I'm asking: would you be open to include a PR that adds this little change?

Let me know, and thank you again!

Conduitry commented 6 months ago

Hey! There are a number of things here that, were I writing this package today, I would probably do differently. The 'detect whether Blob exists as a global and either return a Blob or Buffer' thing seems, in hindsight, like a bad idea. I haven't checked all the implications of doing this, but what right now is sounding like a better idea is to just have a single exported function that returns a Uint8Array and then leave it to the consumer to convert that to a Buffer or to a Blob depending on their use case. How does that sound to you?

Conduitry commented 6 months ago

Also, in the meantime, if the issue for your users is Buffer as an implicit global, then I think you ought to be able to use this library's toArray and then call Buffer.from() on your end with Buffer appropriately imported. This would move you in the direction you would have to go anyway with my above proposal.

alexandercerutti commented 5 months ago

Hey @Conduitry, first of all thanks for your reply!

I see what you mean, of course after 5 years you see things with a completely different eye!

I think you ought to be able to use this library's toArray and then call Buffer.from() on your end with Buffer appropriately imported

That is an interesting solution that could help to bypass everything. Thank you!

There are a number of things here that, were I writing this package today, I would probably do differently.

About this, I've also added Typescript definitions to DefinitelyTyped some years ago. Another thing to improve, could be to import them in here, in order to have just one source for typings.

The 'detect whether Blob exists as a global and either return a Blob or Buffer' thing seems, in hindsight, like a bad idea [...] a better idea is to just have a single exported function that returns a Uint8Array and then leave it to the consumer to convert that to a Buffer or to a Blob depending on their use case

I like this. Checking Blob to perform a choice is a poor choice to me either. I think using a Uint8Array would be the best choice because, as you might already know, a Buffer is already an extension of Uint8Array in Node.

So, using a Uint8Array would give you an implicit support to all the JS runtimes and platforms out there that follow the standard (both Cloudflare workers / pages and Node included but not limited to).

Then, correct me if I'm wrong, if you would remove the check "Blob || Buffer", that would mean to deprecate and remove everything but toArray and either include it in the index.js, as it doesn't even make sense anymore to have a dedicated file.

alexandercerutti commented 5 months ago

Is there anything I can do to help here? If you think I can help, I perhaps could setup a PR with the changes you said. Let me know :)