Skip to content

fix(gatsby-source-contentful): Correct supported image formats#29562

Merged
ascorbic merged 1 commit intomasterfrom
fix/contentful-image-catch-invalid
Feb 18, 2021
Merged

fix(gatsby-source-contentful): Correct supported image formats#29562
ascorbic merged 1 commit intomasterfrom
fix/contentful-image-catch-invalid

Conversation

@ascorbic
Copy link
Copy Markdown
Contributor

Contentful doesn't support AVIF. This fixes the resolver, and adds a check in the url builder

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Feb 18, 2021
@ascorbic ascorbic force-pushed the fix/contentful-image-catch-invalid branch from 94cfe3e to 628edce Compare February 18, 2021 10:05
@ascorbic ascorbic force-pushed the fix/contentful-image-catch-invalid branch from 628edce to 2ce9ce4 Compare February 18, 2021 10:06
@ascorbic ascorbic requested a review from axe312ger February 18, 2021 10:08
Copy link
Copy Markdown
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

I've added a small nit.

// cache is more likely to go stale than the images (which never go stale)
// Note that the same image might be requested multiple times in the same run

const validImageFormats = new Set([`jpg`, `png`, `webp`])
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.

Not sure why a set is needed here.

Suggested change
const validImageFormats = new Set([`jpg`, `png`, `webp`])
const validImageFormats = [`jpg`, `png`, `webp`]

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 was under the impression that set.has was significantly faster than array.includes

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.

It probably is but it doesn't matter on a 3 items list. For me when I read Sets I expect things to get added to it and making sure things are unique and that makes it more complex in my head.

Sets are fine, that's why I approved it but wanted to get the reason on the why :)

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.

It's just a pattern I always use for this sort of check, particularly when dealing with potentially large numbers of nodes, with a constant Set.

height = CONTENTFUL_IMAGE_MAX_SIZE
}

if (!validImageFormats.has(toFormat)) {
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.

Suggested change
if (!validImageFormats.has(toFormat)) {
if (!validImageFormats.includes(toFormat)) {

@ascorbic ascorbic added topic: source-contentful Related to Gatsby's integration with Contentful and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Feb 18, 2021
@ascorbic ascorbic merged commit 3b4d32f into master Feb 18, 2021
@ascorbic ascorbic deleted the fix/contentful-image-catch-invalid branch February 18, 2021 10:43
ascorbic added a commit that referenced this pull request Feb 18, 2021
@ascorbic
Copy link
Copy Markdown
Contributor Author

Published in gatsby-source-contentful@4.6.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: source-contentful Related to Gatsby's integration with Contentful

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants