Skip to content

Return fallback image when remote server returns 404#355

Merged
eduardoboucas merged 5 commits intodevelopfrom
feature/fallback-from-remote
May 10, 2018
Merged

Return fallback image when remote server returns 404#355
eduardoboucas merged 5 commits intodevelopfrom
feature/fallback-from-remote

Conversation

@jimlambie
Copy link
Copy Markdown
Contributor

This PR replicates the functionality of S3 and Disk storage where, if configured, a fallback image is returned to the consumer instead if just a JSON message.

Close #353

@jimlambie jimlambie requested a review from eduardoboucas May 9, 2018 23:51
Copy link
Copy Markdown
Contributor

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

🎉

Comment thread dadi/lib/index.js
let address = this.address()
let env = config.get('env')

/* istanbul ignore next */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👏

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👏

Comment thread dadi/lib/storage/http.js

return reject(err)
if (err.statusCode === 404) {
return new Missing().get().then(stream => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I personally tend to not create Promise constructs when already inside one, which would allow you to get rid of the catch:

return resolve(
  new Missing().get().then(stream => {
    this.notFound = true
    this.lastModified = new Date()

    return stream
  })
)

But this one is really just a matter of personal taste, so more than happy for it to stay as it is!

Comment thread test/acceptance/controller.js Outdated
@@ -1,21 +1,22 @@
var AWS = require('aws-sdk-mock')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you mean to set this one as var?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oops

Comment thread test/acceptance/controller.js Outdated
client
.get('/jpg/50/0/0/801/478/0/0/0/2/aspectfit/North/0/0/0/0/0/testxxx.jpg')
.end(function (err, res) {
let isBuffer = (res.body instanceof Buffer)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this be just res.body.should.be.instanceof(Buffer)? 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I knew there was a better way!

Comment thread dadi/lib/index.js
let address = this.address()
let env = config.get('env')

/* istanbul ignore next */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👏 👏

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👏 👏

Comment thread dadi/lib/index.js
var address = this.address()
let env = config.get('env')

/* istanbul ignore next */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👏 👏 👏

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👏 👏 👏

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

😂 😹 😂

Comment thread README.md

[![npm (scoped)](https://img.shields.io/npm/v/@dadi/cdn.svg?maxAge=10800&style=flat-square)](https://www.npmjs.com/package/@dadi/cdn)
[![coverage](https://img.shields.io/badge/coverage-86%25-yellow.svg?style=flat)](https://github.com/dadi/cdn)
[![coverage](https://img.shields.io/badge/coverage-88%25-yellow.svg?style=flat)](https://github.com/dadi/cdn)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This makes me so happy!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📈

@eduardoboucas eduardoboucas merged commit 3b739d2 into develop May 10, 2018
@jimlambie jimlambie deleted the feature/fallback-from-remote branch June 6, 2018 08:55
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.

3 participants