feat(contentful): add traced SVGs to Contentful images #5659

axe312ger · 2018-06-01T11:18:21Z

Since Contentful is supported by the SQIP transformer, this now enables traced SVG previews for Contentful Assets :)

gatsbybot · 2018-06-01T11:24:31Z

Deploy preview for using-drupal ready!

Built with commit 7d6b311

https://deploy-preview-5659--using-drupal.netlify.com

gatsbybot · 2018-06-01T11:26:14Z

Deploy preview for gatsbygram ready!

Built with commit 7d6b311

https://deploy-preview-5659--gatsbygram.netlify.com

axe312ger · 2018-06-01T11:31:39Z

axe312ger · 2018-06-01T11:36:04Z

using-contentful deployment is failing since it can't find the new fragments. I am pretty sure this is because it is not published yet.

BUT

I am getting the same error on my local installation. The gif above I actually worked because I did not use the fragment, I put the required values there on my own. Any ideas why my Fragments are not found by Gatsby?

1:31:33 PM: error GraphQLCompilerContext: Cannot find fragment `GatsbyContentfulSizes_tracedSVG`. Please make sure the fragment exists in `GatsbyContentfulSizes`.
1:31:34 PM: 
1:31:34 PM:   Error: GraphQLCompilerContext: Cannot find fragment `GatsbyContentfulSizes_tra  cedSVG`. Please make sure the fragment exists in `GatsbyContentfulSizes`.

https://app.netlify.com/sites/using-contentful/deploys/5b112d828df894600842a8f4

m-allanson · 2018-06-01T13:16:44Z

It's caused by these lines I think:

gatsby/packages/gatsby-source-contentful/src/gatsby-node.js

Lines 214 to 217 in 31530b8

    
           await fs.copy( 
        
             require.resolve(`gatsby-source-contentful/src/fragments.js`), 
        
             `${program.directory}/.cache/fragments/contentful-asset-fragments.js` 
        
           )

The fragments are copied from your local Gatsby install's src directory into your project's .cache directory. When using gatsby-dev, it copies only the compiled js files to your local Gatsby install , not the src files.

So the above lines are copying the fragments into your project's .cache directory from the published version of Gatsby instead of your local development version.

axe312ger · 2018-06-01T13:51:20Z

Alright. So I'd say this is ready for review/release? <3

KyleAMathews · 2018-06-01T15:09:36Z

Nice!

pieh · 2018-06-01T15:53:19Z

The fragments are copied from your local Gatsby install's src directory into your project's .cache directory.

How about copying built fragments.js then? What's the reason for copying it from src (which cause problems like this)? (this could also be changed then in gatsby-transformer-sharp which copies fragments.js from src too)

There is also issue of download images - right now this is duplicated in sqip plugin, and there is PR from @Khaledgarbaya ( https://github.com/gatsbyjs/gatsby/pull/5542/files/ba9cef15ee44be31c85c6f121fc2029bbe9b9c8f#diff-64f3938da48c0307f60ef3360a76d68fR49 ) that adds this in yet another place. Problem I have with this sqip and this PR have separate caches and It would be nice to abstract it, so this can be reused in all places I mentioned (not true blocker, but would be nice to solve it)

axe312ger · 2018-06-02T12:04:11Z

How about copying built fragments.js then? What's the reason for copying it from src (which cause problems like this)? (this could also be changed then in gatsby-transformer-sharp which copies fragments.js from src too)

Makes totally sense. Should be a seperate PR which maybe gets merged before this to fix deployment preview 🤔

There is also issue of download images - right now this is duplicated in sqip plugin

I agree, we should have a generic function which all plugins use for this

Problem I have with this sqip and this PR have separate caches and It would be nice to abstract it, so this can be reused in all places I mentioned (not true blocker, but would be nice to solve it)

Same here, maybe gatsby should define a folder where such files should be cached. Not all downloaded files need to end up in public/static. Maybe .cache/files/[plugin-name]. This also touches #5662

pieh · 2018-06-02T12:49:17Z

Same here, maybe gatsby should define a folder where such files should be cached. Not all downloaded files need to end up in public/static. Maybe .cache/files/[plugin-name]. This also touches #5662

We do have createRemoteFileNode which handles downloads, caching but also creates file nodes which won't really work here - if this would be broken down so we could import just downloading and caching part this could be solution here

axe312ger · 2018-06-04T09:24:31Z

@pieh

So these are the Contentful image download duplicates:

I see two options:

Make a generic function to download Contentful images and cache them outside of public/static
Make a generic function to download any file and cache them outside of public/static

Both should return a Gatsby file object since in many cases these structure is needed. Others use cases can just extract the values they need.

What do you think?

Related to #5542

axe312ger · 2018-06-04T13:40:48Z

I created #5698 which should get https://deploy-preview-5659--using-contentful.netlify.com/image-api/ deployed successfully after it was merged :)

pieh · 2018-06-05T16:20:37Z

So these are the Contentful image download duplicates:

https://github.com/axe312ger/gatsby/blob/3b5d355bf739dc767505cd02f9280f4afa6c109a/packages/gatsby-source-contentful/src/extend-node-type.js#L288-L342

https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-transformer-sqip/src/extend-node-type.js#L198-L248

https://github.com/Khaledgarbaya/gatsby/blob/ba9cef15ee44be31c85c6f121fc2029bbe9b9c8f/packages/gatsby-remark-images-contentful/src/index.js#L45-L53

As Khaled pointed out in there - it doesn't actually download and save file, so we should skip it from discussion (my bad for bringing this up).

I see two options:

Make a generic function to download Contentful images and cache them outside of public/static

Make a generic function to download any file and cache them outside of public/static

Both should return a Gatsby file object since in many cases these structure is needed. Others use cases can just extract the values they need.

What do you think?

As I mentioned above there is createRemoteFileNode (used by some source plugins - like wordpress/drupal to download media library) - we could refactor it a bit so we could use just downloading and caching part of it. In situation like this we don't really want to create Gatsby File node because at this stage (doing work in resolver) schema is already created. It shouldn't break anything to just use it in its current form and extract path to downloaded file from it - it just would do some unnecessary work/overhead).

I can implement it first in sqip plugin and then you could "copy" this to this PR - would do you think about that?

axe312ger · 2018-06-06T09:54:42Z

@pieh sounds wonderful! We might consider just adding a new flag like non-permanent so it won't store it to the Gatsby db but does everything else. However, I am sure you will do it properly :)

pieh · 2018-06-07T13:35:05Z

packages/gatsby-source-contentful/src/extend-node-type.js

+    // Downloading small version of the image with same aspect ratio
+    const assetWidth = maxWidth || width
+    const assetHeight = maxHeight || height
+    let aspectRatio = image.aspectRatio


I was testing this locally and image.aspectRatio was never defined for me - had to use this:

let aspectRatio = image.file.details.image.height / image.file.details.image.width

what am I missing here?

pieh · 2018-06-07T13:37:48Z

Just posting progress update - I started working on using createRemoteFileNode utility but hit road block: currently it doesn't handle query params, so will need to handle that first.

pieh · 2018-06-07T14:53:09Z

packages/gatsby-source-contentful/package.json

@@ -33,5 +33,8 @@
    "build": "babel src --out-dir . --ignore __tests__",
    "prepublish": "cross-env NODE_ENV=production npm run build",
    "watch": "babel -w src --out-dir . --ignore __tests__"
+  },
+  "optionalDependencies": {
+    "gatsby-plugin-sharp": "^1.6.46"


If this is set as optional there should be some kind of check in code if it's available before using it. I think this should be just moved to dependencies

Lets just move it, everybody working with images should have that one anyways.

pieh · 2018-06-07T14:54:39Z

My current thinking is to merge this almost as-is so switching to using createRemoteFileNode don't block it. Only other changes I would like (not counting those in inline comments) is to not use const { tmpdir } = require(os) and download images to .cache rather than system tmp. Sounds good?

axe312ger · 2018-06-07T17:28:30Z

Sounds good to me,

Switching to .cache makes sense, wanted to do the very same with the sqip images.

Why would you not use const { tmpdir } = require(os)?

axe312ger · 2018-06-11T14:10:51Z

packages/gatsby-transformer-sqip/src/extend-node-type.js

-async function sqipSharp({ type, cache, getNodeAndSavePathDependency }) {
+async function sqipSharp({ type, cache, getNodeAndSavePathDependency, store }) {
+  const program = store.getState().program
+  const cacheDir = resolve(`${program.directory}/.cache/sqip/`)


Hint: This changes the cacheDir of SQIP to be aligned with the cached Contentful assets. Both of these do not need to be publicly available.

axe312ger · 2018-06-11T14:11:50Z

@pieh @m-allanson @KyleAMathews This is ready for review.

IMHO the PR cleans up the code a lot and adds features while increasing readability :)

pieh · 2018-06-12T12:57:02Z

This looks good - I have just 2 semi related issues:

we need to split this into 2 PRs - move using-contentful changes to separate PR to avoid problems with preview and not yet published plugin (I can manage this one)
tracedSVG examples uses same images as default sizes example which mean images will be already loaded when user get to tracedSVG section and visitors won't be able to see tracedSVG placeholders - maybe we can merge them together? examples you added use sizes just with extra tracedSVG field

axe312ger · 2018-06-12T13:15:49Z

Done, I did split it up into two PRs. See #5859

axe312ger · 2018-06-12T13:17:56Z

I incremented the size by one pixel to ensure the user will see the traced SVG :) (In the other PR)

Anything else? 🤝

pieh

Thank you for keeping up with my comments and getting this across the finish line! ❤️

axe312ger · 2018-06-12T14:59:38Z

🎉

nimaiwalsh · 2018-06-13T08:29:02Z

Awesome work!

Sorry for being a n00b, but can someone please provide an example query to include an image sourced from Contentful using the traced image?

I have only used imageSharp with a local file like below in the past.

imageSharp(id: { regex: "/bg.jpeg/" }) {
      sizes(maxWidth: 1240) {
        ...GatsbyImageSharpSizes
      }
  }

Many thanks

pieh · 2018-06-13T08:37:55Z

@nimaiwalsh Hey, changes from this PR weren't published yet. I will leave comment when they will go live.
When we publish new release we will also merge https://github.com/gatsbyjs/gatsby/pull/5859/files that include example how to use traced image with contentful images!

nimaiwalsh · 2018-06-13T08:42:22Z

@pieh Awesome. Thanks.

* master: (26 commits) Publish [feat] Extract internal plugin for automatically creating pages so we can reuse for other directories (#4490) fix(contentful): properly delete deleted entries and assets (#5756) Fix typo add Linda's post (#5817) [gatsby-source-contentful] Fix prepareJSONNode for array normalization (#5107) Process Contentful images inlined in markdown (#5542) feat(contentful): add traced SVGs to Contentful images (#5659) [SQIP] Add documentation (#5287) Add site to showcase (#5819) [gatsby-remark-copy-linked-files] Support reference-style images (#5818) Update docs for gatsby-plugin-catch-links (#5843) Added ⋅ representing a space to sub-list examples to reflect actual code (#5829) Adding tigerfacilityservices.com to showcase (#5855) fix schema type conflict reporter regressions (#5805) [gatsby-plugin-feed] Support nested output directory (#5820) Make cache available to the plugins (#5840) [gatsby-source-contentful] update data and jest snapshots (#5802) [gatsby-plugin-manifest] Replace all manifest.json with manifest.webmanifest in comments/docs (#5157) Update deploy-gatsby.md (#5800) ... # Conflicts: # docs/tutorial/part-one/index.md # examples/using-sqip/src/components/polaroid.js # packages/gatsby-image/README.md # packages/gatsby-image/package.json # packages/gatsby-plugin-catch-links/package.json # packages/gatsby-plugin-feed/package.json # packages/gatsby-plugin-manifest/package.json # packages/gatsby-plugin-page-creator/src/gatsby-node.js # packages/gatsby-remark-copy-linked-files/package.json # packages/gatsby-source-contentful/package.json # packages/gatsby-source-contentful/src/__tests__/__snapshots__/normalize.js.snap # packages/gatsby-source-contentful/src/extend-node-type.js # packages/gatsby-source-contentful/src/gatsby-node.js # packages/gatsby-source-contentful/src/normalize.js # packages/gatsby-transformer-remark/package.json # packages/gatsby-transformer-remark/src/__tests__/__snapshots__/gatsby-node.js.snap # packages/gatsby-transformer-sqip/package.json # packages/gatsby-transformer-sqip/src/extend-node-type.js # packages/gatsby/package.json # packages/gatsby/src/bootstrap/load-plugins/__tests__/__snapshots__/load-plugins.js.snap # packages/gatsby/src/bootstrap/load-plugins/load.js # www/src/data/sites.yml

axe312ger mentioned this pull request Jun 4, 2018

fix(fragments): copy fragments from build files to easy up developing #5698

Closed

m-allanson mentioned this pull request Jun 5, 2018

Copy fragments from build files to ease up developing #5728

Closed

pieh reviewed Jun 7, 2018

View reviewed changes

feat(contentful): add traced SVGs to Contentful images

d026d0c

axe312ger force-pushed the feat/contentful-tracedsvg branch from 54abaf1 to c1c57e0 Compare June 11, 2018 14:09

axe312ger commented Jun 11, 2018

View reviewed changes

Benedikt Rötsch added 2 commits June 12, 2018 15:14

docs(contentful): add traced SVG explanation to gatsby-image readme

3a79ecd

refactor(contentful): add function to download & cache contentful images

7d6b311

axe312ger force-pushed the feat/contentful-tracedsvg branch from c1c57e0 to 7d6b311 Compare June 12, 2018 13:14

axe312ger mentioned this pull request Jun 12, 2018

add tracedsvg feature to using contentful #5859

Merged

pieh approved these changes Jun 12, 2018

View reviewed changes

pieh merged commit f327863 into gatsbyjs:master Jun 12, 2018

axe312ger deleted the feat/contentful-tracedsvg branch June 12, 2018 14:59

tylermenezes mentioned this pull request Jun 15, 2018

gatsby-source-contentful - mirroring images locally? #5919

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(contentful): add traced SVGs to Contentful images #5659

feat(contentful): add traced SVGs to Contentful images #5659

axe312ger commented Jun 1, 2018

gatsbybot commented Jun 1, 2018 •

edited

Loading

gatsbybot commented Jun 1, 2018 •

edited

Loading

axe312ger commented Jun 1, 2018

axe312ger commented Jun 1, 2018

m-allanson commented Jun 1, 2018 •

edited

Loading

axe312ger commented Jun 1, 2018

KyleAMathews commented Jun 1, 2018

pieh commented Jun 1, 2018 •

edited

Loading

axe312ger commented Jun 2, 2018

pieh commented Jun 2, 2018

axe312ger commented Jun 4, 2018

axe312ger commented Jun 4, 2018

pieh commented Jun 5, 2018

axe312ger commented Jun 6, 2018

pieh Jun 7, 2018 •

edited

Loading

pieh commented Jun 7, 2018

pieh Jun 7, 2018

axe312ger Jun 7, 2018 •

edited

Loading

pieh commented Jun 7, 2018

axe312ger commented Jun 7, 2018

axe312ger Jun 11, 2018

axe312ger commented Jun 11, 2018

pieh commented Jun 12, 2018

axe312ger commented Jun 12, 2018

axe312ger commented Jun 12, 2018

pieh left a comment

axe312ger commented Jun 12, 2018

nimaiwalsh commented Jun 13, 2018

pieh commented Jun 13, 2018

nimaiwalsh commented Jun 13, 2018

feat(contentful): add traced SVGs to Contentful images #5659

feat(contentful): add traced SVGs to Contentful images #5659

Conversation

axe312ger commented Jun 1, 2018

gatsbybot commented Jun 1, 2018 • edited Loading

gatsbybot commented Jun 1, 2018 • edited Loading

axe312ger commented Jun 1, 2018

axe312ger commented Jun 1, 2018

m-allanson commented Jun 1, 2018 • edited Loading

axe312ger commented Jun 1, 2018

KyleAMathews commented Jun 1, 2018

pieh commented Jun 1, 2018 • edited Loading

axe312ger commented Jun 2, 2018

pieh commented Jun 2, 2018

axe312ger commented Jun 4, 2018

axe312ger commented Jun 4, 2018

pieh commented Jun 5, 2018

axe312ger commented Jun 6, 2018

pieh Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

pieh commented Jun 7, 2018

pieh Jun 7, 2018

Choose a reason for hiding this comment

axe312ger Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

pieh commented Jun 7, 2018

axe312ger commented Jun 7, 2018

axe312ger Jun 11, 2018

Choose a reason for hiding this comment

axe312ger commented Jun 11, 2018

pieh commented Jun 12, 2018

axe312ger commented Jun 12, 2018

axe312ger commented Jun 12, 2018

pieh left a comment

Choose a reason for hiding this comment

axe312ger commented Jun 12, 2018

nimaiwalsh commented Jun 13, 2018

pieh commented Jun 13, 2018

nimaiwalsh commented Jun 13, 2018

gatsbybot commented Jun 1, 2018 •

edited

Loading

gatsbybot commented Jun 1, 2018 •

edited

Loading

m-allanson commented Jun 1, 2018 •

edited

Loading

pieh commented Jun 1, 2018 •

edited

Loading

pieh Jun 7, 2018 •

edited

Loading

axe312ger Jun 7, 2018 •

edited

Loading