[source-contentful] If entry is not set by user, provide an empty value of the same type #2037

m4rrc0 · 2017-09-06T16:26:37Z

quick and dirty solution to allow 'blank' fields in contentful.
Sorry for the noise on closing commas. Prettier stepped in.

Would love a review and some tests on different data sets.
@Khaledgarbaya your input is more than welcome too. ;)

m4rrc0 · 2017-09-06T16:29:11Z

trying to address #1517

gatsbybot · 2017-09-06T16:36:16Z

Deploy preview ready!

Built with commit 34a67cf

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

Khaledgarbaya

Looks great, just a few comments.
Can you add some unit test to make sure you didn't miss an edge case, like localized fields etc?
Also maybe @KyleAMathews can give more input?

Khaledgarbaya · 2017-09-07T07:44:35Z

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

+            entryItemFields[fieldName] = ""
+          } else if (contentTypeItemField.type.match(/Number/)) {
+            // NOTE: Might be a problem for some people to auto set to 0?
+            entryItemFields[fieldName] = 0


since Number(undefined) === NaN let's set the default to NaN

good point. Thanks

KyleAMathews

Yeah other than missing tests, looks very reasonable and mergeable!

KyleAMathews · 2017-09-07T07:48:37Z

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

@@ -111,7 +111,7 @@ exports.buildForeignReferenceMap = ({
                }
                foreignReferenceMap[v.sys.id].push({
                  name: `${contentTypeItemId}___NODE`,
-                  id: entryItem.sys.id,
+                  id: entryItem.sys.id


Please run "npm run format" from the root of the repo

ooh so that's the trick! ok thanks

KyleAMathews · 2017-09-07T07:57:35Z

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

+            entryItemFields[fieldName] = ""
+          } else if (contentTypeItemField.type.match(/Number/)) {
+            // NOTE: Might be a problem for some people to auto set to 0?
+            entryItemFields[fieldName] = 0


m4rrc0 · 2017-09-07T08:23:06Z

Of course I forgot about tests! I am not familiar with the test workflow so I might be coming back with questions. I'll try and dive in today or tomorrow.
Thanks for the feedacks

Khaledgarbaya · 2017-09-07T12:23:25Z

@MarcCoet you can check the existing tests for the plugin and add to it

dan-weaver · 2017-09-07T15:22:25Z

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

+          ) {
+            entryItemFields[fieldName] = {}
+          } else if (contentTypeItemField.type.match(/Array/)) {
+            entryItemFields[fieldName] = []


something that could be problematic I guess (it is for me at least) is that [] and {} are truthy, and these fields are obviously nullable in Contentful or we wouldn't be here :). I have code like this { data.someField || "Doesn't exist" } and I imagine that's pretty common. Anyway, I still think this is better than what I'm doing now (making sure the field exists in a placeholder entity in contentful).

m4rrc0 · 2017-09-08T10:13:48Z

dan-weaver's comment made me dig a little more and a few problems showed up. Please bare with me because I'll need guidance if we want to take this a step further.

(Context: I am still investigating jest so this is purely based on trial and error)
While this seems to be working fairly well when a few values are missing, it is not the case when a specific entry is never filled in.
Let's take a simple example: a content model for blog post would have an entry for tags (an array of strings).

if there is at least one tag in at least one blog post, it works. Moreover I can apparently set blank values to null instead of [] and everything is fine.
if no tag is set in any blog post then it fails. But it will work if set to [""] (which definitely makes dan-weaver -and any developer for that matter- unhappy ^^). -> The error raised is that we query an unknown field.

While this is a somehow relatively working solution for arrays of strings, it miserably fails for assets, arrays of assets, references (with blank fields), arrays of references, ...

IF we want to go that way and set empty fields inside empty fields (?) then my question is: is the sync API handling an "include" parameter? It does not seem so and even though I am not super familiar with the Contentful API I guess this might be a problem because we can't see easily inside Link entries...

With a more global concern in mind, I am wondering why we don't just authorize developers to query fields that does not exist. I mean if I want to be a punk and query a bunch of stuff that does not exist in the schema, I would gladly see a big fat warning in the console but then go on with the rest of the query. That way, if I expect that query to be valid at some point in the future it is already in place.
My question here is more regarding Gatsby Core I think: Is it possible (and desirable) to simply set to null any part of any query that does not exist (and raise a big fat warning)?
Now it raises

GraphQL Error Unknown field '...' on type '...'

and makes everything fail. Does it really have to be that drastic @KyleAMathews ?

Back to the problem at hand: I still think this PR handles part of the problem so I'll get back to my tests ASAP.

m4rrc0 · 2017-09-08T17:44:49Z

I feel like the best way to implement tests is to add empty fields to the demo space and update the snapshot. I updated the current snapshot but there are only blank fields on strings and arrays of strings.

m4rrc0 · 2017-09-08T17:51:18Z

packages/gatsby-source-contentful/src/__tests__/__snapshots__/normalize.js.snap

        "type": "ContentfulBrand",
      },
      "logo___NODE": "c4zj1ZOfHgQ8oqgaSKm4Qo2",
      "node_locale": "en-US",
      "parent": "Brand",
+      "phone": Array [
+        "",
+      ],


Is it something we want or should I leave it like an empty array (please see my long comment above for details on why I changed that)

Yeah we'd need this or else the schema would be wrong e.g. Gatsby wouldn't know that phone is an array of strings.

KyleAMathews · 2017-09-11T07:44:16Z

I am wondering why we don't just authorize developers to query fields that does not exist.

This could be desirable perhaps but GraphQL says otherwise so you'd need to take the question to them :-)

…rom one entry to another

m4rrc0 · 2017-09-17T18:02:59Z

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

+        if (contentTypeItemField.type === `Object`) {
+          if (typeof entryItemFields[fieldName].json === `undefined`) {
+            entryItemFields[fieldName].json = JSON.stringify(
+              entryItemFields[fieldName]


this is a proposition to solve #1703

instead of converting every object to string, I added a json property to every object containing the stringified version of the object. This means we can query that json field on any object even if the object content vary from entry to entry.
Does that make sense?
Should we choose a wiser property name?

m4rrc0 · 2017-09-18T12:15:16Z

I would like to add support for non existant entries, meaning if there is e.g. a blogpost content type but no entry of that type is created in contentful then we can still query those blogposts and get 'blank values' for each field.
Is there a way to add the blogpost queryType to the schema without actually creating an entry of type blogPost (and would that make sense) @KyleAMathews ?

lsirivong · 2017-11-22T23:23:27Z

What's the status of this? Any way I can help carry forward?

sedubois · 2017-11-23T08:26:22Z

Contrary to the PR title, this concerns actually all source types, not just contentful. Right now (from memory), e.g. if cloning the gatsby website and removing some field from all blog posts (e.g. imageTitle), it will break the build with an incomprehensible error message.

There should ideally be a way to intercept the node creation to add any needed default values to a specific GraphQL type.

sedubois · 2017-11-23T08:36:13Z

This is especially needed for auto-generated field types, such as the blog post excerpt used in the gatsbyjs.org blog.

m4rrc0 · 2017-11-23T12:35:11Z

@sedubois you are right. From what I've seen the most interesting discussion on the topic is #2392.
@lsirivong I am using the code suggested in this PR in a local copy of the contentful plugin and it does the job for me until a 'real' solution comes to life.

KyleAMathews · 2017-12-11T18:53:33Z

@lsirivong pinged me about how to get this in — I think conceptually it's sound — we want code which double checks that the entire schema is translated correctly from Contentful to Gatsby & fills in missing pieces.

What I'd like improved here is there needs to be a lot more tests as this is a tricky thing to do correctly so we need to ensure things are solid.

KyleAMathews · 2017-12-11T19:42:06Z

this concerns actually all source types, not just contentful

@sedubois totally agree — but this isn't solvable (directly) unfortunately in most source plugins. Contentful is unique in that we can get access to the actual schema for different content types where on e.g. gatsbyjs.org — there is no defined schema other than the data we add to various files.

One way solve this (worth pursuing on another issue) is Gatsby let you attach schemas perhaps written in GraphQL's IDL language to source plugins to use for their schemas to replace the inferred schema.

hirviid · 2017-12-17T09:48:59Z

@MarcCoet @KyleAMathews What is currently holding you back from merging this? There should be more tests? Can I do anything to help?

jtschulz · 2017-12-27T16:37:41Z

We have an interest in this for our gatsby/ contentful integration as well. Depending what changes are needed, we could lend one of our developers to carry this work over the line in the next couple of weeks. Just let us know @KyleAMathews @MarcCoet

m4rrc0 · 2017-12-27T18:31:41Z

@hirviid @PepperTeasdale thanks for stepping in. From what I understood it is lacking some tests and a way to add them could be to complexify the data that is being used to create the test snapshot.

If you have time or ressource available to work on this, you may want to find a better solution than this one though. As Kyle said above, the best solution might be to infer the GraphQL schema from Contentful content types.
Another interesting idea to generalize this approach might be #3344

sarahatwork · 2018-01-03T21:51:36Z

I want to bring up one more complex use case that's come up in my project. We have one model called a ContentPage with a modules field that can link to many other types of models like BodyText, InlineImage, and Chart.

This allows for flexible, modular pages. The graphql looks like this:

modules {
  __typename
  ... on ContentfulBodyText {
    #fields specific to BodyText
  }
  ... on ContentfulInlineImage {
    #fields specific to InlineImage
  }
}

Given this structure and the fact that I have about 10 of these module types, my build currently fails unless each of these models is used at least once on a ContentPage. So if an editor decides to unpublish the only carousel, the site goes down.

I looked into extending this PR to deal with complex relations like this but hit a dead end. Not only would I need to create a placeholder BodyText - I'd have to create a relationship on both sides (ContentPage to BodyText, and BodyText to ContentPage). I wasn't able to get this working and wound up giving up on this approach. I think using snapshots or writing graphql from scratch, as discussed on #3344, will be more successful.

Or maybe my schema is just too complicated :)

m4rrc0 · 2018-01-04T08:37:39Z

@sarahatwork best way I found to avoid that is create in advance an instance of every content type and link them together at every possible place. I called all these entries 'IGNORE' and told editors to... ignore them. Then in my queries, I do the same.
filter: { name: { ne: "IGNORE" } }
At least the content types exist so the build succeed.

I am also in favor of the #3344 approach as it will allow far more opportunities and it is CMS agnostic.

Khaledgarbaya · 2018-01-04T10:38:55Z

👋 @sarahatwork ~~that's how graphql works, it encourages to type everything and I really like that because it makes sure that you really know what you are querying IMO we should not have a workaround there.~~

sarahatwork · 2018-01-04T14:13:44Z

@MarcCoet Yeah I'm moving ahead with a dummy entry approach now while waiting for a real fix. Instead of ruling out by name I've made a new DummyContentIndex content type that just links to all the other dummy items. This way, I can filter out any item that has a dummycontentindex property.

Khaledgarbaya · 2018-01-04T15:13:19Z

Hey @sarahatwork I think I was a bit confused. is the modules field one single ref field or multiple references?

sarahatwork · 2018-01-04T15:19:16Z

@Khaledgarbaya It's the type "References, many" in Contentful. Here's one in action on one of our content pages:

Khaledgarbaya · 2018-01-04T15:45:43Z

ok, I totally misunderstood you in the previous comment, sorry about that, I am also in favor of #3344 to solve the problem once and for all.

KyleAMathews · 2018-04-04T00:47:11Z

Closing this as we all agree #3344 is a better approach.

Khaledgarbaya suggested changes Sep 7, 2017

View reviewed changes

KyleAMathews suggested changes Sep 7, 2017

View reviewed changes

dan-weaver reviewed Sep 7, 2017

View reviewed changes

m4rrc0 commented Sep 8, 2017

View reviewed changes

m4rrc0 added 4 commits September 17, 2017 19:14

If entry is not set by user, provide an empty value of the same type

7b29ced

avoid validationError on createTextNode

d8d9227

NaN for Numbers. Recursive function for arrays. Format

462b092

add json property on objects to allow query even if properties vary f…

34a67cf

…rom one entry to another

m4rrc0 force-pushed the topics/contentful-source-from-content-model branch from 300de3e to 34a67cf Compare September 17, 2017 17:53

m4rrc0 commented Sep 17, 2017

View reviewed changes

m4rrc0 mentioned this pull request Oct 11, 2017

Allow non-existing fields in GraphQL Queries #2392

Closed

daniel-simpson mentioned this pull request Nov 11, 2017

Coalesce field values to ensure no broken production build on no content #2882

Closed

sarahatwork mentioned this pull request Dec 15, 2017

[gatsby-source-contentful] Add stringified internal.content prop to JSON objects #2967

Merged

KyleAMathews closed this Apr 4, 2018

This was referenced Mar 22, 2024

[Snyk] Security upgrade sharp from 0.20.8 to 0.24.1 philipedin/gatsby#1584

Open

[Snyk] Security upgrade sharp from 0.20.8 to 0.24.1 philipedin/gatsby#1592

Open

[source-contentful] If entry is not set by user, provide an empty value of the same type #2037

[source-contentful] If entry is not set by user, provide an empty value of the same type #2037

Conversation

m4rrc0 commented Sep 6, 2017

m4rrc0 commented Sep 6, 2017

gatsbybot commented Sep 6, 2017 • edited Loading

Khaledgarbaya left a comment • 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

KyleAMathews left a 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

m4rrc0 commented Sep 7, 2017

Khaledgarbaya commented Sep 7, 2017

Choose a reason for hiding this comment

m4rrc0 commented Sep 8, 2017

m4rrc0 commented Sep 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KyleAMathews commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4rrc0 commented Sep 18, 2017 • edited Loading

lsirivong commented Nov 22, 2017

sedubois commented Nov 23, 2017

sedubois commented Nov 23, 2017

m4rrc0 commented Nov 23, 2017

KyleAMathews commented Dec 11, 2017

KyleAMathews commented Dec 11, 2017 • edited Loading

hirviid commented Dec 17, 2017

jtschulz commented Dec 27, 2017

m4rrc0 commented Dec 27, 2017

sarahatwork commented Jan 3, 2018 • edited Loading

m4rrc0 commented Jan 4, 2018

Khaledgarbaya commented Jan 4, 2018 • edited Loading

sarahatwork commented Jan 4, 2018

Khaledgarbaya commented Jan 4, 2018

sarahatwork commented Jan 4, 2018

Khaledgarbaya commented Jan 4, 2018

KyleAMathews commented Apr 4, 2018

gatsbybot commented Sep 6, 2017 •

edited

Loading

Khaledgarbaya left a comment •

edited

Loading

m4rrc0 commented Sep 18, 2017 •

edited

Loading

KyleAMathews commented Dec 11, 2017 •

edited

Loading

sarahatwork commented Jan 3, 2018 •

edited

Loading

Khaledgarbaya commented Jan 4, 2018 •

edited

Loading