-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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
Fixes #1194: OEmbed http requests use "request" npm package #2555
Conversation
…f official node modules.
Thanks @nishimaki10 |
@nishimaki10 awesome, but I have some questions. Why not use the Meteor's HTTP module http://docs.meteor.com/#/full/http_call ? Is there some way to limit the request size? Users can past URL of big files and we don't wan't to download the entire file to get only the headers. |
@rodrigok Thanks for review! I have used for the Meteor first time. I did not know that exists Meteor's HTTP module. Thank you for the valuable information. But, may not be able to implement limit the request size. We have to control the request stream. It's probably like this. I looking at this, it is impossible to touch the stream. What should I do? |
@nishimaki10 If you can limit the request using the lib |
@rodrigok I see. I will try to implement using the |
And use internal request module.
stream.abort() | ||
|
||
stream.on 'end', Meteor.bindEnvironment -> | ||
buffer = Buffer.concat(chunks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the gunzip? The library do the unzip for each chunk? I don't think so, can you test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The request module pipe gunzip for each chunk. see also. I think that this library is made well.
My Test:
gziped site
Screenshot:
Twitter, GitHub, etc, etc... I tested many sites. All OK. |
LGTM |
@nishimaki10 sorry about that, but, can you fix the conflicts? |
@rodrigok OK, solved. |
Closes #1194
OEmbed http requests use "request" npm package instead of official node modules.