minio / minio-js

MinIO Client SDK for Javascript
https://docs.min.io/docs/javascript-client-quickstart-guide.html
Apache License 2.0
928 stars 273 forks source link

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

Open aldy505 opened 9 months ago

aldy505 commented 9 months ago

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.

prakashsvmx commented 9 months ago

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.

trim21 commented 9 months ago

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

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

trim21 commented 9 months ago

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

aldy505 commented 9 months ago

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?

prakashsvmx commented 9 months ago

@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.

aldy505 commented 9 months ago

@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