Skip to content
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

🙈 Used got to handle requests for image-size #8953

Merged
merged 5 commits into from
Oct 9, 2017

Conversation

aileen
Copy link
Member

@aileen aileen commented Aug 30, 2017

closes #8589
refs #8868

This PR ports most of the changes of #8892 and #8986 to improve the image-size util.


  • Swapped request with got in getImageSizeFromUrl fn.
  • wrapped got request library in its own request.js util that returns bluebird promises and validates URL before starting a request
  • Used catch predicates instead of conditionals in getImageSizeFromUrl
  • Returned NotFoundError if applicable as the caller function cachedImageSizeFromUrl is differentiating those between this error and others.
  • The logic that checked for an existing protocol (e. g. gravatar URLs) was overly complicated. Refactored it to be more simple.
  • We're always resolving the result in getCachedImageSizeFromUrl, so there's no need to return the values with a Promise.resolve(). The caller fn uses waits for the Promises to be fulfilled.
  • added and improved tests

@aileen aileen added the LTS label Aug 30, 2017
@aileen aileen added this to the Upcoming LTS release milestone Aug 30, 2017
refs TryGhost#8589
refs TryGhost#8868

Swap `request` with `got` in `getImageSizeFromUrl` fn.
@aileen aileen force-pushed the lts-refactor-image-size-use-got branch from c604e1b to f26bb9d Compare August 31, 2017 08:07
@kirrg001 kirrg001 removed this from the Upcoming LTS release milestone Sep 8, 2017
no issue

This PR includes a new util which wraps the `got` library. It is not used in the codebase yet, but tested with `image-size` util:
- wraps `got` request library in its own `request.js` util that returns bluebird promises and validates URL before starting a request
- adds tests
no issue

- swapped the usage of `got` for requests with the request util
@aileen aileen force-pushed the lts-refactor-image-size-use-got branch 4 times, most recently from 9aab8fb to 653665b Compare September 13, 2017 06:30
no issue

- Used catch predicates instead of conditionals in `getImageSizeFromUrl`
- Returned `NotFoundError` if applicable as the caller function `cachedImageSizeFromUrl` is differentiating those between this error and others.
- The logic that checked for an existing protocol (e. g. gravatar URLs) was overly complicated. Refactored it to be more simple.
- added more tests
no issue

We're always resolving the result in `getCachedImageSizeFromUrl`, so there's no need to return the values with a `Promise.resolve()`. The caller fn uses waits for the Promises to be fulfilled.
@aileen aileen force-pushed the lts-refactor-image-size-use-got branch from 653665b to aee0ac4 Compare September 13, 2017 06:49
@kirrg001
Copy link
Contributor

kirrg001 commented Oct 6, 2017

@AileenCGN Can you pls PR the AMP bump to LTS?

@kirrg001
Copy link
Contributor

kirrg001 commented Oct 9, 2017

Will test all edge cases on staging before the public release.

@kirrg001 kirrg001 merged commit b1d829d into TryGhost:lts Oct 9, 2017
@aileen aileen deleted the lts-refactor-image-size-use-got branch October 13, 2017 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants