-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(DataResolver): prefer streams over buffers #4075
feat(DataResolver): prefer streams over buffers #4075
Conversation
Add `resolveFileAsBuffer` to use it in `resolveImage` which still requires Buffers to work.
Uh, don't merge, getting |
Some progress on the const https = require('https');
const FormData = require('form-data');
const fetch = require('node-fetch');
const httpbin = async (key, value) => {
const body = new FormData();
body.append(key, value);
const res = await fetch('http://httpbin.org/post', { body, method: 'POST' });
console.log(await res.json());
};
(async () => {
const url = 'https://discord.js.org/index.html';
// works, Content-Length header is empty
await httpbin('index.html', await new Promise(ok => https.get(url, ok)));
// doesn't work, Content-Length is being set to some numeric value
await httpbin('index.html', await fetch(url).then(r => r.body));
})(); |
Alright, found it: form-data/form-data#382 fixes the issue I'm having here in this PR. Unfortunately, it doesn't seem like it will be merged in near future. In a nutshell: there's a bug in form-data so it can't handle all streams in the same way. Streams returned by |
Please describe the changes this PR makes and why it should be merged:
I've noticed high memory usage when tried to send a few ~100mb files as message attachments so I went to the source code and discovered that, indeed, everything is being converted to Buffers for some reason. Since discord.js uses node-fetch under the hood, which accepts Streams, the fix seems to be pretty simple.
Status
Semantic versioning classification: