minio-js icon indicating copy to clipboard operation
minio-js copied to clipboard

Remove `web-encoding` dependency and use Nodejs built-in TextEncoder

Open aldy505 opened this issue 1 year ago • 6 comments

Since we're only supporting major new LTS versions[1] and we're just using TextEncoder on our web-encoding dependency, we might as well drop the dependency and just use Node.js' built-in TextEncoder. It should serve the same purpose like the one we currently use.

Request for comments.

[1] See discussion here: https://github.com/minio/minio-js/pull/1236#discussion_r1397407506 -- Node.js' TextEncoding has been around since Node.js v8 and stable as global object since Node.js v11.

aldy505 avatar Nov 22 '23 07:11 aldy505

This npm package was updated to work both in nodejs and web browser environment. as of now, (broken) it does not work in browser environment, we need to check and fix once migration is complete.

prakashsvmx avatar Nov 22 '23 07:11 prakashsvmx

I think we don't need web-encoding or Nodejs built-in TextEncoder.

https://github.com/minio/minio-js/pull/1246#discussion_r1402006530

trim21 avatar Nov 22 '23 13:11 trim21

TextEncoder is for encoding utf-8 string to Uint8Array, which can be done by node's Buffer.from

trim21 avatar Nov 22 '23 13:11 trim21

TextEncoder is for encoding utf-8 string to Uint8Array, which can be done by node's Buffer.from

@trim21 @prakashsvmx regarding the browser environment support: do we have a specific build (other than regular CJS and ESM) that targets Browser and converts every Node dependency into browser supported objects?

aldy505 avatar Nov 22 '23 13:11 aldy505

@aldy505 , we need to explore and investigate . because modern bundlers/builds do not export node built-ins for web and not all modules are ported (browserify) for web.

prakashsvmx avatar Nov 23 '23 04:11 prakashsvmx

@aldy505 , we need to explore and investigate . because modern bundlers/builds do not export node built-ins for web and not all modules are ported (browserify) for web.

Well, on browser, we already have https://developer.mozilla.org/en-US/docs/Web/API/TextEncoder and on Node.js, it's already on the global scope and stable since v11 https://nodejs.org/api/globals.html#textencoder

aldy505 avatar Nov 24 '23 01:11 aldy505