Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't expect application/json type nodes to be files #8544

Merged
merged 4 commits into from
Sep 26, 2018

Conversation

coreyward
Copy link
Contributor

gatsby-source-contentful creates nodes with a JSON type so we can't expect all nodes to have a node.dir property to generate a GraphQL type from.

Fixes #8538.

@coreyward coreyward requested a review from eLod September 26, 2018 00:06
@@ -10,6 +10,8 @@ async function onCreateNode({ node, actions, loadNodeContent, createNodeId }, pl
return pluginOptions.typeName
} else if (isArray) {
return _.upperFirst(_.camelCase(`${node.name} Json`))
} else if (node.internal.type !== `File`) {
return `${node.internal.type} Json`
Copy link
Contributor

Choose a reason for hiding this comment

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

i am not sure if this is the better option, as this dictates the graphql schema, e.g. the contentful sets internal.type to _.camelCase(`${node.internal.type} ${key} JSONNode`) so you would need to query by that name. do you have examples from contentful? what about node.name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is node.name always available and suitable for GraphQL? I have no dog in the fight, just trying to get the incompatibility resolved so I can keep moving on a project.

Copy link
Contributor

Choose a reason for hiding this comment

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

well the type gets transformed to a valid graphql name, so when node.dir is "letter" node.internal.type is "Letter Json" and you can query letterJson (and allLetterJson).node.name is already used so i guess it's safe to use in this if branch as well. that said maybe we should let @pieh chime in.

Copy link
Contributor

Choose a reason for hiding this comment

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

node.name is also File specific - but at least it won't crash if name is not set, but yeah, let's move this else if before else if (isArray)

@eLod
Copy link
Contributor

eLod commented Sep 26, 2018

cc @pieh

@coreyward
Copy link
Contributor Author

@pieh Thanks for the assistance, and for tests!

@pieh pieh merged commit cd780aa into master Sep 26, 2018
@pieh pieh deleted the fix-transformer-json-with-contentful branch September 26, 2018 14:32
lipis added a commit to lipis/gatsby that referenced this pull request Sep 26, 2018
* 'master' of github.com:gatsbyjs/gatsby:
  feat(cache-dir): Add a button for custom 404s in development (gatsbyjs#8548)
  fix: add ecosystem team for starters YAML
  fix: fix displaying and hydrating 404 page in production builds (gatsbyjs#8510)
  fix: don't expect `application/json` type nodes to be files (gatsbyjs#8544)
  Delete cds-takeaways.md (gatsbyjs#8552)
  chore(release): Publish
  chore: remove npm-run-all dep and update deps in e2e tests (gatsbyjs#8529)
  [www] Site/Starter Showcase refactor, rinse II (gatsbyjs#8411)
  Rfc process doc (gatsbyjs#8537)
  [www] a11y improvements (gatsbyjs#8536)
  Update the GraphQL stitching blog post & docs (gatsbyjs#8516)
  fix: change teams to reduce noise for people
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