gatsby-transformer-json incompatible with gatsby-source-contentful

[coreyward]

Description

I believe there's an incompatibility between gatsby-transformer-json and gatsby-source-contentful that results in an error on boot. I am experiencing this issue identically to #5680 (closed, marked stale).

In short, gatsby-transformer-json/gatsby-node.js attempts to process JSON coming back from gatsby-source-contentful and fails when it's unable to determine the directory that the JSON is in.

Steps to reproduce

1. Open up the Contentful example app
2. Install dependencies (yarn) and run develop
3. Confirm it works
4. yarn add gatsby-transformer-json
5. Add gatsby-transformer-json to gatsby-config.js
6. Attempt to run develop

Expected result

The app behaves identically.

Actual result

success open and validate gatsby-config — 0.009 s
success load plugins — 0.140 s
success onPreInit — 0.365 s
success delete html and css files from previous builds — 0.010 s
info One or more of your plugins have changed since the last time you ran Gatsby. As
a precaution, we're deleting your site's cache to ensure there's not any stale
data
success initialize cache — 0.015 s
success copy gatsby files — 0.025 s
success onPreBootstrap — 0.003 s
⠁ Starting to fetch data from Contentful
Fetching default locale
⠠ source and transform nodesdefault locale is : en-US
⠂ source and transform nodescontentTypes fetched 6
Updated entries  12
Deleted entries  0
Updated assets  11
Deleted assets  0
Fetch Contentful data: 558.445ms
error Plugin gatsby-transformer-json returned an error

  Error: TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Rece  ived type undefined
  
  - gatsby-node.js:40 getType
    [using-contentful]/[gatsby-transformer-json]/gatsby-node.js:40:49
  
  - gatsby-node.js:82 Object.<anonymous>
    [using-contentful]/[gatsby-transformer-json]/gatsby-node.js:82:113
  
  - Generator.next
  
  - util.js:16 tryCatcher
    [global]/[bluebird]/js/release/util.js:16:23

error Plugin gatsby-transformer-json returned an error

  Error: TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Rece  ived type undefined
  
  - gatsby-node.js:40 getType
    [using-contentful]/[gatsby-transformer-json]/gatsby-node.js:40:49
  
  - gatsby-node.js:82 Object.<anonymous>
    [using-contentful]/[gatsby-transformer-json]/gatsby-node.js:82:113
  
  - Generator.next
  
  - util.js:16 tryCatcher
    [global]/[bluebird]/js/release/util.js:16:23
  
error UNHANDLED REJECTION

  TypeError: Cannot read property 'filter' of undefined
  
  - api-runner-node.js:274 Promise.mapSeries.catch.then.results
    [using-contentful]/[gatsby]/dist/utils/api-runner-node.js:274:42
  
  - util.js:16 tryCatcher
    [using-contentful]/[bluebird]/js/release/util.js:16:23

(note: stacks truncated)

Environment

System:
OS: macOS High Sierra 10.13.6
Binaries:
Node: 10.9.0 - /usr/local/bin/node
Yarn: 1.9.4 - /usr/local/bin/yarn
npm: 6.2.0 - /usr/local/bin/npm
npmPackages:
gatsby: ^2.0.0 => 2.0.8
gatsby-image: ^2.0.5 => 2.0.9
gatsby-plugin-google-analytics: ^2.0.5 => 2.0.6
gatsby-plugin-offline: ^2.0.5 => 2.0.5
gatsby-plugin-typography: ^2.2.0 => 2.2.0
gatsby-source-contentful: ^2.0.1 => 2.0.1
gatsby-transformer-json: ^2.1.1 => 2.1.1
gatsby-transformer-remark: ^2.1.1 => 2.1.3

[coreyward]

I'm able to get around this for my particular situation with this diff, but I don't know the broader implications of relying on the availability of contentDigest as a test for “should this be processed by transformer-json". Hoping someone more senior on the project can weigh in and advise:

diff --git a/packages/gatsby-transformer-json/src/gatsby-node.js b/packages/gatsby-transformer-json/src/gatsby-node.js
index d7d55243d..fe369a6c0 100644
--- a/packages/gatsby-transformer-json/src/gatsby-node.js
+++ b/packages/gatsby-transformer-json/src/gatsby-node.js
@@ -38,7 +38,10 @@ async function onCreateNode({ node, actions, loadNodeContent, createNodeId }, pl
   const { createNode, createParentChildLink } = actions
 
   // We only care about JSON content.
-  if (node.internal.mediaType !== `application/json`) {
+  if (
+    node.internal.mediaType !== `application/json` ||
+    node.internal.contentDigest
+  ) {
     return
   }
 

[pieh]

node.internal.contentDigest is expected to never be falsy, so this change would disable creating json nodes for any content.

Seems like error comes from here

gatsby/packages/gatsby-transformer-json/src/gatsby-node.js

Line 14 in 7955fbb

return _.upperFirst(_.camelCase(`${path.basename(node.dir)} Json`))

It expects that json comes from file (node.dir would be there if it was). I would think that adding yet another else if (yay, more custom logic) before

gatsby/packages/gatsby-transformer-json/src/gatsby-node.js

Line 11 in 7955fbb

} else if (isArray) {

with something like

else if (node.internal.type !== `File`) {
  return `${node.internal.type} Json`
}

would potentially fix this

[eLod]

this is caused by path.basename(node.dir) in https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-transformer-json/src/gatsby-node.js#L14, i added the getType function, but the functionality hasn't changed (for the specific code path we are talking about), so i guess it was broken before as well. maybe check if node.dir exists and fallback to something else? what @pieh suggested may also work.

edit: we could also just use node.name as the array branch does (though that was for file content originally as well)

[coreyward]

Thanks @pieh, @eLod. If I understand @pieh's proposal, this would still have gatsby-transformer-json run on Contentful JSON. Is there any benefit to that? Would it be better/worse to short circuit here?

gatsby/packages/gatsby-transformer-json/src/gatsby-node.js

Lines 41 to 43 in 4173eb0

	if (node.internal.mediaType !== `application/json`) {
	return
	}

[pieh]

This would also short circuit all other non-file usage of json transformer and make it unusable for those. We need to make a change anyway to fix build breakage, so I would vote for making it work with non-file jsons.

Can you access your json data when short circuiting it?

[eLod]

i am not familiar with contentful, but i see it creates nodes with internal.mediaType = "application/json" but the actual "content" of the node is already parsed js object, see https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-source-contentful/src/normalize.js#L187 so yeah maybe it should be also excluded from gatsby-transformer-json, like checking the node's owner

[pieh]

That's weird way of doing that on contentful plugin side then - if it does handle json, then it probably shouldn't create node with application/json mime (but changing that right now would probably be breaking change).

It seems like contentful is only plugin in this repo that is creating nodes with that mime, but I don't know about 3rd party plugins - I still think we should fix type name generation to handle non-files sources, to be sure we don't break any other existing plugins and avoid referencing other plugins if possible (checking if node was created by contentful plugin)

[coreyward]

@pieh Okay, README of transformer-json suggests it only works with files. We should update that to make broader usage clear.

Yes, I can access my JSON data in data/pricing.json with the short-circuit in place.

@eLod I thought so too, but if you look the next line does JSON.stringify:

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

Line 188 in f14c3b4

const str = JSON.stringify(content)

[eLod]

@pieh right, agreed, what i've meant is in addition to fixing the obvious bug with node.dir also fixing the double json parsing with contentful (in transformer-json by checking the owner, or something similar). that said i am not familiar enough with contentful to know where that json is parsed originally, e.g. if it should be fixed there (like keeping it text and defer parsing to transformer-json for example).

@coreyward well that is for internal.content and internal.contentDigest the keys and values from content are assigned to the root of the created node in https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-source-contentful/src/normalize.js#L190

[coreyward]

@eLod Oh, gotcha. It's turtles all the way down. 😅

Pull request coming with the node.internal.type !== "File" logic branch in getType.

[eLod]

@coreyward right but that does not solve the double parsing problem. i guess contentful shouldn't parse the json itself it that's viable and rely on transformer-json as it does with markdown content i guess.

[coreyward]

@eLod Yep, not trying to fix two issues in one PR. In any case I think the three of us would need additional context for the rationale behind the Contentful behavior and I'm hoping to get this PR in ASAP so I can use it without referencing a fork in my dependencies.

[eLod]

@coreyward you can also set the typeName option for gatsby-transformer-json to a string or a function so that problematic code path does not get triggered. it does not solve the bug, but let's you progress without forking/modifying anything else.

edit: e.g. see https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-transformer-json#configuration-options