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

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

Merged
merged 3 commits into from
Nov 28, 2017

Conversation

sarahatwork
Copy link
Contributor

This is a potential solution to #1703. The issue is that right now you can only query JSON data types if you know all the keys.

This PR adds an internal.content property to nodes with type Object with a stringified version of the object. With this, we're supporting both uses cases:

  1. Where you know the keys of your JSON object and want to query them directly
  2. Where you don't know (or don't want to keep track of) the keys and simply want all of the object's data

My use case is that I'm using a JSON object in a translations field to store translations for my site.
It's entered in Contentful like this (kindly ignore my placeholder Chinese 🙈 ):

screenshot 2017-11-19 13 32 14

I don't want to have to update my queries every time I add a new translation, so being able to grab a stringified property works best for me.

Here's all the data (explicit keys and internal.content) on Graphiql:

screenshot 2017-11-19 13 49 29

Another common use case for JSON objects where this solution might help is to store library settings.

One issue 🛑: I updated the data on packages/gatsby-source-contentful/src/__tests__/data.json manually with some data from my app. Before this is merged, it would be best to add a JSON field in the Gatsby test Contentful space, run the download script (to regenerate data.json), and update the test snapshots.

@gatsbybot
Copy link
Collaborator

gatsbybot commented Nov 19, 2017

Deploy preview ready!

Built with commit 2e06c19

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

@gatsbybot
Copy link
Collaborator

gatsbybot commented Nov 19, 2017

Deploy preview ready!

Built with commit 2e06c19

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

Copy link
Contributor

@Khaledgarbaya Khaledgarbaya left a comment

Choose a reason for hiding this comment

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

LGTM, Great job 👏

@KyleAMathews
Copy link
Contributor

Sorry for the slow reply — was out most of last week for Thanksgiving.

I added a new content model w/ a json field and created a new content item w/ the package.json for Gatsby :-) I thought that appropriate.

@sarahatwork if you'd like to redownload the content and update the test this seems ready to go!

@sarahatwork
Copy link
Contributor Author

@KyleAMathews Thanks for making the update - all set!

@KyleAMathews
Copy link
Contributor

Awesome! Merging + publishing

@KyleAMathews KyleAMathews merged commit 11d34e3 into gatsbyjs:master Nov 28, 2017
@sarahatwork sarahatwork deleted the topic/contentful-json branch November 28, 2017 21:42
@mrtnpro
Copy link

mrtnpro commented Dec 15, 2017

Thanks for tackling this issue @sarahatwork! I'm using JSON object fields and ran into that just yesterday. A quick update to [email protected] solved it. However I just ran into another edge case:

When your JSON object field actually contains a collection...

screenshot 2017-12-14 19 46 02

... which according to contentful is valid. However gatsby-contentful-source fails with a similar error message:

error UNHANDLED REJECTION


  Error: Names must match /^[_a-zA-Z][_a-zA-Z0-9]*$/ but "0___price" does not.
  
  - Array.map
  
  - create-sort-field.js:19 createSortField
    [App]/[gatsby]/dist/schema/create-sort-field.js:19:20
  
  - build-node-connections.js:62 
    [App]/[gatsby]/dist/schema/build-node-connections.js:62:16
  
  - lodash.js:537 arrayEach
    [App]/[lodash]/lodash.js:537:11
  
  - lodash.js:9359 Function.forEach
    [App]/[lodash]/lodash.js:9359:14
  
  - build-node-connections.js:33 module.exports
    [App]/[gatsby]/dist/schema/build-node-connections.js:33:5
  
  - index.js:42 _callee$
    [App]/[gatsby]/dist/schema/index.js:42:25
 

/cc @KyleAMathews

@sarahatwork
Copy link
Contributor Author

@crtvhd Ah, good find. Unfortunately I don't think I'll be able to look into this (if I have spare time, #2037 is a more pressing issue to my current project). Maybe in the case of an array we'd want to manually nest it under a property - something like:

// before
[ { "foo": "bar" } ]

// after
{
  [name of field]: [ { "foo": "bar" } ]
}

@flaviolivolsi
Copy link
Contributor

flaviolivolsi commented Apr 24, 2018

The issue is located in the line gatsby-source-contentful/src/normalize.js:172 inside the prepareJSONNode function, which spreads the content of the JSON whether it's an array or a json. As @sarahatwork says nesting solves it, but it also creates a breaking change for who's relying on the current structure.

My solution for a fix that doesn't break anything is nesting the JSON only if it's an array. I'll shoot a PR soon.

--EDIT--

I sent a PR here

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.

6 participants