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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃檲 Used got to handle requests for image-size #8892

Merged
merged 1 commit into from
Aug 31, 2017
Merged

馃檲 Used got to handle requests for image-size #8892

merged 1 commit into from
Aug 31, 2017

Conversation

aileen
Copy link
Member

@aileen aileen commented Aug 14, 2017 •
edited

refs #8589
refs #8868

Swap request with got in getImageSizeFromUrl fn.

TODOs:

@aileen aileen force-pushed the refactor-image-size-use-got branch from 2e5e2bf to 6934a22 Compare August 14, 2017 11:42
aileen
aileen commented Aug 14, 2017
View reviewed changes
core/test/unit/server_helpers/ghost_head_spec.js
@@ -42,7 +42,7 @@ describe('{{ghost_head}} helper', function () {
return localSettingsCache[key];
});

configUtils.set('url', 'http://localhost:82832/');
configUtils.set('url', 'http://localhost:65530/');

This comment was marked as abuse.

Sign in to view

@aileen aileen changed the title [WIP] Used got to handle requests for image-size 馃檲 Used got to handle requests for image-size Aug 14, 2017
@aileen
Copy link
Member Author

aileen commented Aug 14, 2017

@ErisDS: I assume this change needs a lot of testing, as our image size util is the cause for many dispatcher issues. If you feel like we're missing some more tests, please let me know.

@aileen
Copy link
Member Author

aileen commented Aug 14, 2017

Also, I didn't test yet, if this PR solves the issues in #8885 and #8589.

@aileen aileen force-pushed the refactor-image-size-use-got branch 2 times, most recently from f097088 to 3cbdb1d Compare August 15, 2017 11:32
@guoyk93
Copy link

guoyk93 commented Aug 17, 2017

@AileenCGN @ErisDS I made a small work around PR #8915 for issue #8885, just handle the situation when 'icon' is a full url.

ErisDS
ErisDS requested changes Aug 17, 2017
View reviewed changes
Copy link
Member

@ErisDS ErisDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking so much better! I think there is a last pass through to do to tidy up one or two more things :)

core/server/utils/image-size-from-url.js Outdated
@@ -34,7 +31,7 @@ var sizeOf = require('image-size'),
module.exports.getImageSizeFromUrl = function getImageSizeFromUrl(imagePath) {
return new Promise(function imageSizeRequest(resolve, reject) {

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

core/server/utils/image-size-from-url.js Outdated
requestOptions
).then(function (response) {
try {
dimensions = sizeOf(response.body);

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

core/server/utils/image-size-from-url.js Outdated
code: 'IMAGE_SIZE',
statusCode: err.statusCode,
context: err.url
}));

This comment was marked as abuse.

Sign in to view

core/server/utils/image-size-from-url.js Outdated
message: 'Image not found.',
code: 'IMAGE_SIZE',
statusCode: err.statusCode,
context: err.url

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@aileen aileen force-pushed the refactor-image-size-use-got branch from 3cbdb1d to 5958a4f Compare August 18, 2017 05:02
@aileen aileen mentioned this pull request Aug 29, 2017
Merged
2 tasks
@aileen aileen force-pushed the refactor-image-size-use-got branch from 5958a4f to d4fa95d Compare August 29, 2017 04:39
@kirrg001
Copy link
Contributor

I've tested several cases from the past and looks like got is doing a good job.
e.g. follow redirects, timeouts

@aileen aileen force-pushed the refactor-image-size-use-got branch from d4fa95d to f52279e Compare August 30, 2017 10:10
@aileen
Used got to handle requests for image-size
87edb67 
refs TryGhost#8589
refs TryGhost#8868

Swap `request` with `got` in `getImageSizeFromUrl` util.
@aileen aileen force-pushed the refactor-image-size-use-got branch from f52279e to 87edb67 Compare August 31, 2017 07:25
@kirrg001 kirrg001 merged commit 30bee11 into TryGhost:master Aug 31, 2017
This was referenced Sep 8, 2017
Closed
Merged
kirrg001 pushed a commit that referenced this pull request Oct 9, 2017
@aileen @kirrg001
馃悰 Fixed following redirects for image dimension requests (#8953)
b1d829d 
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 deleted the refactor-image-size-use-got branch October 13, 2017 08:31
@ErisDS ErisDS removed their assignment Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ErisDS ErisDS ErisDS requested changes

@kirrg001 kirrg001 kirrg001 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aileen @guoyk93 @kirrg001 @ErisDS