search

rkazakov / ampify

Convert your HTML to Google AMP (Accelerated Mobile Pages)
MIT License
109 stars 32 forks source link

Request is async #16

Closed gruberjl closed 2 years ago

gruberjl commented 6 years ago

request is asynchronous. It doesn't look like the code supports the promise properly. We'll need to convert to promises, async/await, or use a package like sync-request.

Let me know which you'd prefer and I'll send a pull request.

rkazakov commented 6 years ago

Good point, @gruberjl! How about using something like axios or request-promise to convert to promises?

bkniffler commented 6 years ago

Any plans for a fix? I can't run ampify without getting errors due to request('GET', imageUrl) not being a valid use of request library (should be request.get(imageUrl), but then again the async issue needs to be fixed). Wouldn't it be better, at this point, to just let this part of functionality be commented out until fixed?

bkniffler commented 6 years ago

I added a PR here: https://github.com/rkazakov/ampify/pull/21

rkazakov commented 6 years ago

Thanks for your contribution, @bkniffler! Will you be able to use promises instead of async/await as we want to keep Node support prior to version 8? Also can I ask you to add unit test coverage for your code please?