✨ Replace http and https with got #101

aileen · 2017-09-13T09:44:36Z

closes #99

Used got for image-size requests, which is more lightweight and has the huge advantage of following redirects
Added and updated test
indents
Added dependencies:
- validator
- lodash
- got

coveralls · 2017-09-13T09:46:30Z

Coverage decreased (-1.8%) to 96.46% when pulling 9638035 on AileenCGN:use-got-and-follow-redirects into 94ac029 on jbhannah:master.

coveralls · 2017-09-13T10:00:03Z

Coverage decreased (-0.02%) to 98.23% when pulling ebaccdf on AileenCGN:use-got-and-follow-redirects into 94ac029 on jbhannah:master.

coveralls · 2017-09-13T10:01:15Z

Coverage decreased (-0.02%) to 98.23% when pulling ebaccdf on AileenCGN:use-got-and-follow-redirects into 94ac029 on jbhannah:master.

kirrg001 · 2017-10-03T07:51:24Z

package.json

    "htmlparser2": "3.9.2",
    "image-size": "0.5.1",
+    "lodash": "4.17.4",


In sub dependencies/libraries we should always use the caret.

See one line below, the lib required single lodash functions. Either we continue or you can remove the extra line.

kirrg001 · 2017-10-03T07:55:02Z

lib/amperize.js

-      var options = url.parse(element.attribs.src),
-          timeout = 5000,
-          request = element.attribs.src.indexOf('https') === 0 ? https : http;
+      var imagePath = url.parse(element.attribs.src),


For readability: This is not a path, this is an object.

Makes sense!

kirrg001 · 2017-10-03T08:09:24Z

lib/amperize.js


      called = false;

+      if (!validator.isURL(imagePath.href)) {
+        if (called) return;


Why do we have to keep the called variable?
I see three obvious cases:

invalid url

success

error

Can you please explain?

I wasn't sure if this is still needed when switching to got. I removed it now, as got should be able to handle it properly.

kirrg001 · 2017-10-03T08:14:29Z

test/amperize.test.js

@@ -310,7 +384,7 @@ describe('Amperize', function () {
            .get('/images/IMG_xyz.jpg')
            .socketDelay(5500)
            .reply(200, {
-                data: '<Buffer 2c be a4 40 f7 87 73 1e 57 2c c1 e4 0d 79 03 95 42 f0 42 2e 41 95 27 c9 5c 35 a7 71 2c 09 5a 57 d3 04 1e 83 03 28 07 96 b0 c8 88 65 07 7a d1 d6 63 50>'
+                body: '<Buffer 2c be a4 40 f7 87 73 1e 57 2c c1 e4 0d 79 03 95 42 f0 42 2e 41 95 27 c9 5c 35 a7 71 2c 09 5a 57 d3 04 1e 83 03 28 07 96 b0 c8 88 65 07 7a d1 d6 63 50>'


can handle timeout errors

If i run all tests locally, this test does not wait 5s, it finishes immediately.

It's supposed to do so, afaik. We only pretend that there's a timeout.

That does not make sense to me. How do we ensure the timeout works? 🤔
I guess you need to use .delay and increase the test timeout with this.timeout.

Because the requested server returns a timeout error. This is what we mock.

If we want to test, if got library handles timeout, then we can use .delay I think?

Ok. Understood what you meant. I changed the test to simulate a real timeout with .delay and this.timeout().

coveralls · 2017-10-03T08:35:27Z

Coverage decreased (-0.2%) to 98.095% when pulling 71349aa on AileenCGN:use-got-and-follow-redirects into 65d7037 on jbhannah:master.

coveralls · 2017-10-03T09:05:00Z

Coverage decreased (-0.2%) to 98.095% when pulling b15df2f on AileenCGN:use-got-and-follow-redirects into 65d7037 on jbhannah:master.

coveralls · 2017-10-03T10:13:19Z

Coverage decreased (-0.2%) to 98.095% when pulling f1ea202 on AileenCGN:use-got-and-follow-redirects into 65d7037 on jbhannah:master.

closes jbhannah#99 - Used [`got`](https://github.com/sindresorhus/got) for image-size requests, which is more lightweight and has the huge advantage of following redirects - Added and updated test - indents - dercreased timeout to 3s instead of 5s - Added dependencies: - [validator](https://github.com/chriso/validator.js) - [lodash](https://github.com/lodash/lodash) - [got](https://github.com/sindresorhus/got)

coveralls · 2017-10-03T10:37:24Z

Coverage decreased (-0.2%) to 98.095% when pulling c0f10c7 on AileenCGN:use-got-and-follow-redirects into 65d7037 on jbhannah:master.

coveralls · 2017-10-03T10:54:52Z

Coverage increased (+1.8%) to 100.0% when pulling 49e9f89 on AileenCGN:use-got-and-follow-redirects into 65d7037 on jbhannah:master.

aileen force-pushed the use-got-and-follow-redirects branch 2 times, most recently from 77b5880 to ebaccdf Compare September 13, 2017 09:58

aileen mentioned this pull request Sep 19, 2017

Amperize issues TryGhost/Ghost#9022

Closed

4 tasks

kirrg001 reviewed Oct 3, 2017

View reviewed changes

aileen force-pushed the use-got-and-follow-redirects branch from ebaccdf to 71349aa Compare October 3, 2017 08:33

aileen force-pushed the use-got-and-follow-redirects branch from 71349aa to b15df2f Compare October 3, 2017 09:03

aileen force-pushed the use-got-and-follow-redirects branch from b15df2f to f1ea202 Compare October 3, 2017 10:12

kirrg001 approved these changes Oct 3, 2017

View reviewed changes

aileen force-pushed the use-got-and-follow-redirects branch from f1ea202 to c0f10c7 Compare October 3, 2017 10:36

Removed unused error conditionals

49e9f89

kirrg001 merged commit b37c94a into jbhannah:master Oct 3, 2017

aileen deleted the use-got-and-follow-redirects branch May 10, 2018 03:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Replace http and https with got #101

✨ Replace http and https with got #101

aileen commented Sep 13, 2017 •

edited

Loading

coveralls commented Sep 13, 2017 •

edited

Loading

coveralls commented Sep 13, 2017 •

edited

Loading

coveralls commented Sep 13, 2017 •

edited

Loading

kirrg001 Oct 3, 2017

kirrg001 Oct 3, 2017

kirrg001 Oct 3, 2017

aileen Oct 3, 2017

kirrg001 Oct 3, 2017

aileen Oct 3, 2017

kirrg001 Oct 3, 2017

aileen Oct 3, 2017

kirrg001 Oct 3, 2017

aileen Oct 3, 2017

aileen Oct 3, 2017

aileen Oct 3, 2017

coveralls commented Oct 3, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading

✨ Replace http and https with got #101

✨ Replace http and https with got #101

Conversation

aileen commented Sep 13, 2017 • edited Loading

coveralls commented Sep 13, 2017 • edited Loading

coveralls commented Sep 13, 2017 • edited Loading

coveralls commented Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 3, 2017 • edited Loading

coveralls commented Oct 3, 2017 • edited Loading

coveralls commented Oct 3, 2017 • edited Loading

coveralls commented Oct 3, 2017 • edited Loading

coveralls commented Oct 3, 2017 • edited Loading

aileen commented Sep 13, 2017 •

edited

Loading

coveralls commented Sep 13, 2017 •

edited

Loading

coveralls commented Sep 13, 2017 •

edited

Loading

coveralls commented Sep 13, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading