gatsbyjs / gatsby

The best React-based framework with performance, scalability and security built in.
https://www.gatsbyjs.com
MIT License
55.19k stars 10.33k forks source link

[source-contentful] handling undefined values #1517

Closed m4rrc0 closed 6 years ago

m4rrc0 commented 7 years ago

Would be nice to have a way to query an entry field regardless of it being defined in contentful. I mean if a field exists in Contentful but is not required (and empty) I would expect getting undefined or null but now the request to Contentful seems to fail.

TypeError: Cannot read property 'contentfulSettings' of undefined
    at graphql.then.result (/home/marc/code/TOILE/toile.io/gatsby-node.js:120:24)
    at tryCatcher (/home/marc/code/TOILE/toile.io/node_modules/bluebird/js/release/util.js:16:23)
    at Promise._settlePromiseFromHandler (/home/marc/code/TOILE/toile.io/node_modules/bluebird/js/release/promise.js:512:31)
    at Promise._settlePromise (/home/marc/code/TOILE/toile.io/node_modules/bluebird/js/release/promise.js:569:18)
    at Promise._settlePromiseCtx (/home/marc/code/TOILE/toile.io/node_modules/bluebird/js/release/promise.js:606:10)
    at Async._drainQueue (/home/marc/code/TOILE/toile.io/node_modules/bluebird/js/release/async.js:138:12)
    at Async._drainQueues (/home/marc/code/TOILE/toile.io/node_modules/bluebird/js/release/async.js:143:10)
    at Immediate.Async.drainQueues (/home/marc/code/TOILE/toile.io/node_modules/bluebird/js/release/async.js:17:14)
    at runCallback (timers.js:800:20)
    at tryOnImmediate (timers.js:762:5)
    at processImmediate [as _immediateCallback] (timers.js:733:5)
chrisk2020 commented 7 years ago

I noticed this is also the case using [source-wordpress]

KyleAMathews commented 7 years ago

From the stack trace it looks like the error is in your gatsby-node.js file. So the query is succeeding. Correct?

m4rrc0 commented 7 years ago

I have this query in my gatsby-node.js

exports.createPages = ({ graphql, boundActionCreators }) => {
  const { createPage } = boundActionCreators
  return new Promise((resolve, reject) => {
    resolve(
      graphql(
        `
        {
          contentfulSettings {
            url
            name
            < ... more here ... >
          }
        }
        `
      ).then(result => {
        if (result.errors) {
          reject(result.errors)
        }

        const {
          url,
          name,
          < ... more here ... >
        } = result.data.contentfulSettings

...

If I have both the url and name fields defined in contentful, everything works fine. If one of the fields is empty however, then result.data is undefined.

sebastienfi commented 7 years ago

That's true, the query will fail if the field does not exists in the node's GraphQL schema.

Do you expect it to work like a reference to an undefined Javascript's Object property would ? It yes, then I don't think that's Gatsby related, the answer lays into the deep understanding of GraphQL's Core.

m4rrc0 commented 7 years ago

Good point @sebastienfi . Your comment made me dig a little. I did not dug into the plugin code (graphQL is not my bff yet) but from my observations I am guessing we infer node's GraphQL schema from the Contentful entries directly, right? As Contentful does not return empty fields in its response it is then absent from the schema. What if we inferred the schema from the Contentful content model instead so that we are sure that every field present in the content model is callable in Gatsby and if a specific field value is absent from the response we define it as null or undefined or whatever goes well with graphQL. What am I missing?

sebastienfi commented 7 years ago

Contentful does not return empty fields in its response

It seems to me that this is the answer. It is not in the response because Contentful seems to ommit fields which has either null or empty value, so Gatsby and GraphQL both doesn't know it exists.

m4rrc0 commented 7 years ago

My point exactly. So what do you think about inferring the graphQL schema from the content model?

My argument is that Contentful is a CMS. It is meant to be used by editors. If a field is not required in contentful the editor will reasonably understand that the field can be empty. But it cannot as the graphQL query will fail in that case! It seems like a real problem to me.

Is there maybe a way to tell Contentful to provide explicit responses for each field, even undefined ones @Khaledgarbaya ?

@KyleAMathews is it the same issue you were mentionning here #1264?

dan-weaver commented 7 years ago

This just bit me as well. Is #1264 addressing this? Not sure you guys are understanding what @MarcCoet is saying? forgive me if I'm wrong.

But, it seems that if a field is specified in a query that doesn't happen to exist in any of the contenful records returned at that time by the api, then the graphql schema is not able to service that field, because it doesn't know it exists?

So, I'm guessing the schema is currently being build from existing records. Is that not correct? Could the schema be built instead from the content type descriptions themselves: https://www.contentful.com/developers/docs/references/content-delivery-api/#/reference/content-types/content-model . I was having trouble deciphering exactly what was going on. Would love to help out with this!

KyleAMathews commented 7 years ago

Yeah Gatsby builds the schema from the data it has. The plugin does use the content schema to identify markdown fields. Would be happy to take a PR that also uses the content model to identify fields that don't have data yet.

dan-weaver commented 7 years ago

@MarcCoet i'm trying to take a stab at this now, let me know if you've already made progress though.

m4rrc0 commented 7 years ago

Awesome @dan-weaver . No unfortunately August is just a long rush for me. No time to investigate. =/ I'd be glad to test anything you have and report back though.

m4rrc0 commented 7 years ago

And BTW, if you have any thought about #1703 . I'd love to read it.

Khaledgarbaya commented 7 years ago

👋 hey there, Sorry I was off for two weeks, I totally agree on having the schema defined based on the content type and it should be fairly easy to that. we have a labs project that allows you to query the data stored in Contentful using graphql, A schema and value resolvers are automatically generated out of an existing space. Please check the source code because there is a lot of useful stuff there. I will be glad to answer any question related to Contentful, but unfortunately I don't have time to make a PR for that

dan-weaver commented 7 years ago

@Khaledgarbaya thanks. I've been procrastinating.. Will let you know if I have any questions! thanks for the examples

m4rrc0 commented 7 years ago

Hey @dan-weaver , any news from your side? I'll try and get my head around this next week. I've got a few thinks to learn before I can be productive with graphql but I'll definitely have something at some point and it will need a review. ;) I'll let you know when I get there.

iw-dweaver commented 7 years ago

@MarcCoet hey sorry for the lack of update. I tried for a bit but probably need some guidance. Also was having trouble with gatsby's development workflow.. Mosty just nuisances though in that regard.

My biggest question at this point is where should this be done

Options as far as I can tell:

  1. within setFieldsOnNodeTypes
  2. setting these fields during the normal createNode routines in normalize.js

I think I'm lacking some understanding of how gatsby constructs its GraphQL types.

m4rrc0 commented 7 years ago

Maybe @KyleAMathews could give you some clues? Never coded a plugin (yet) so I need a bunch of hours on the problem before I can understand your question. ;)

dan-weaver commented 7 years ago

Yeah that'd be great. I know this probably isn't huge priority, but it would be a good learning experience.. I have some other plugins I'd like to write in the future. Hopefully my question above isn't too vague.

KyleAMathews commented 7 years ago

So probably the best way to "fill in" missing fields in the schema would be @iw-dweaver's (1) option to use setFieldsOnGraphQLNodeType. You'd look at each generated schema and compare the schema against the Contentful model. If there's any missing fields, you add that field with the correct type.

3CordGuy commented 7 years ago

Trying to use contentful as a front-end for my wife's blog. I also can't rely on her to always use every field defined in the content schema. Would be great if there was an intuitive solution for this.

jquense commented 7 years ago

The best way to handle these cases for possibly undefined fields is, as Kyle mentioned, to use the setFieldsOnGraphQLNodeType api to explicitly define the fields so they exist whether or not they are empty

dan-weaver commented 7 years ago

Thanks folks. Just FYI @MarcCoet I'm going to set aside some time to dive into this again today / tomorrow, but don't let me be your blocker if it's urgent. And let me know if you've already got something.

Khaledgarbaya commented 7 years ago

If I am not wrong you can here check the fields of the entry and the fields in the contentType, if one the of the fields does not exist you set it to null or something similar.

dan-weaver commented 7 years ago

@Khaledgarbaya I had been going down that route, but to no avail. See my "option 2 above". Someone please correct me if I'm wrong, because I'd love to know. But from looking at the code, it seems Gatsby would not have enough information about the field type if it's simply given a null value there. As far as I could tell the fields were just being dropped when i did that.

m4rrc0 commented 7 years ago

@dan-weaver just to let you know asap that I think I've got a working solution... I hope I didn't miss something huge because I made it quick and dirty. Basically I went Khaled's way and set a value directly when entryNodes are built. I will post a PR as soon as possible.

Khaledgarbaya commented 7 years ago

@MarcCoet That's Great news, feel free to mention me in the PR and I'll try to help there

HZSamir commented 6 years ago

@MarcCoet Still no progress about his issue?

m4rrc0 commented 6 years ago

Hey @Unforgiven-wanda the PR has been left hanging because we need to implement some tests and I don't know anything about tests. My suggestion was to add problematic cases to the test space but got no answer back. I am running the PR code in a local plugin and it is all good for me (at least for what it was supposed to solve). No progress on images and unused content types (e.g. blogPost content type exists but no blog post is created) though. I think #2518 is mentioning the same kind of issue for images

stefanoverna commented 6 years ago

This single issue is preventing our agency to extensively use Gatsby on every new project.. I would be happy to help proposing a PR but I'm not sure how to proceed :(

KyleAMathews commented 6 years ago

@stefanoverna would love to see you using Gatsby for every project! @pieh is working on a fix in #3344 — perhaps you could chat with him to see if you could help out? He's got a branch he's working on that you could look at.

SeanRoberts commented 6 years ago

Is @pieh's schema solution being looked at as the path forward vs. altering gatsby-source-contentful to generate schemas from the Contentful's content model?

@MarcCoet is there any way to use your untested plugin in the meantime? This is a showstopper for us, so would love to have a solution even if it's a little shaky 🙂

pieh commented 6 years ago

@SeanRoberts complete solution would allow gatsby-source-contentful to use content model to generate schema - my "solution" was just proof of concept that it can feasibly be done and actually work. I decided to just use schema definition language there but it could be expanded to add api for plugins to define schema types programatically.

KyleAMathews commented 6 years ago

Due to the high volume of issues, we're closing out older ones without recent activity. Please open a new issue if you need help!

ErisDS commented 6 years ago

For anyone else stumbling across this it's still open/tracked here: https://github.com/gatsbyjs/gatsby/issues/3344

Khaledgarbaya commented 5 years ago

Hey Folks, I haven't tested this but this might be a p[otential Clean workaround that does not require creating dummy content.

https://medium.com/@Zepro/contentful-reference-fields-with-gatsby-js-graphql-9f14ed90bdf9

sami616 commented 5 years ago

@Khaledgarbaya this works great for optional content types but we quickly run into the issue again if one of those types have optional fields that are undefined

gusliedke commented 5 years ago

This is still an issue

crosumo commented 4 years ago

For me its an issue as well. If i have 300 posts and i dont choose a category (or other kind of field) at least in 1, i will have the issue.

m4rrc0 commented 4 years ago

You should try the gatsby-plugin-schema-snapshot. I personally still have issues with Contentful but we are definitely heading in the right direction.

sonusynup commented 4 years ago

I'm still having issue with graphql schema when I run query against non-existing data(Editor might have unpublish the data from Contentful). Any workaround for it????