[gatsby-plugin-typography] fails to compile when headerComponents include null

[danielkov]

Description

After upgrading from v1 to v2 following the official guide I am witnessing the following issue:

error UNHANDLED REJECTION


  TypeError: Cannot read property 'key' of null

  - render-page.js:1997
    /Users/danielkov/projects/project/public/render-page.js:1997:11

  - Array.sort

  - render-page.js:1996 Object../node_modules/gatsby-plugin-typography/gatsby-ssr.js.exports.onPreRenderHTML
    /Users/danielkov/projects/project/public/render-page.js:1996:18

  - render-page.js:196
    /Users/danielkov/projects/project/public/render-page.js:196:36

  - Array.map

  - render-page.js:191 ./.cache/api-runner-ssr.js.module.exports
    /Users/danielkov/projects/project/public/render-page.js:191:25

  - render-page.js:597 Module.default
    /Users/danielkov/projects/project/public/render-page.js:597:57

  - worker.js:35 Promise
    [project]/[gatsby]/dist/utils/worker.js:35:36

  - debuggability.js:303 Promise._execute
    [project]/[gatsby]/[bluebird]/js/release/debuggability.js:303:9

  - promise.js:483 Promise._resolveFromExecutor
    [project]/[gatsby]/[bluebird]/js/release/promise.js:483:18

  - promise.js:79 new Promise
    [project]/[gatsby]/[bluebird]/js/release/promise.js:79:10

  - worker.js:31 Promise.map.path
    [project]/[gatsby]/dist/utils/worker.js:31:37

  - util.js:16 tryCatcher
    [project]/[gatsby]/[bluebird]/js/release/util.js:16:23

  - map.js:61 MappingPromiseArray._promiseFulfilled
    [project]/[gatsby]/[bluebird]/js/release/map.js:61:38

  - promise_array.js:114 MappingPromiseArray.PromiseArray._iterate
    [project]/[gatsby]/[bluebird]/js/release/promise_array.js:114:31

  - promise_array.js:78 MappingPromiseArray.init
    [project]/[gatsby]/[bluebird]/js/release/promise_array.js:78:10

I realise this is because in the following method:

exports.onPreRenderHTML = function (_ref2) {
  var getHeadComponents = _ref2.getHeadComponents,
      replaceHeadComponents = _ref2.replaceHeadComponents;
  var headComponents = getHeadComponents();
  headComponents.sort(function (x, y) {
    if (x.key === "TypographyStyle") {
      return -1;
    } else if (y.key === "TypographyStyle") {
      return 1;
    }

    return 0;
  });
  replaceHeadComponents(headComponents);
};

the value of headComponents is React.ReactNode[], which can include null as valid values. In my project one of the other plugins injects a null into this array, which shouldn't cause an issue to this plugin in my opinion.

Steps to reproduce

1. Add gatsby-plugin-typography to plugins array.
2. Achieve a null in the headers in some way (e.g.: inside <Helmet/>.
3. Try to compile the site in some way develop || build.

Expected result

It should tolerate null and build normally.

Actual result

TypeError: Cannot read property 'key' of null

Environment

  System:
    OS: macOS 10.14.4
    CPU: (8) x64 Intel(R) Core(TM) i7-7820HQ CPU @ 2.90GHz
    Shell: 5.3 - /bin/zsh
  Binaries:
    Node: 11.10.0 - /var/folders/r5/9r6yq3695tqgy0z3bswzxn8h0000gn/T/yarn--1552498926309-0.6699258377655932/node
    Yarn: 1.13.0 - /var/folders/r5/9r6yq3695tqgy0z3bswzxn8h0000gn/T/yarn--1552498926309-0.6699258377655932/yarn
    npm: 5.7.1 - /usr/local/bin/npm
  Languages:
    Python: 2.7.10 - /usr/bin/python
  Browsers:
    Chrome: 72.0.3626.121
    Firefox: 61.0.2
    Safari: 12.1
  npmPackages:
    gatsby: 2.1.31 => 2.1.31 
    gatsby-plugin-catch-links: 2.0.13 => 2.0.13 
    gatsby-plugin-feed: 2.0.15 => 2.0.15 
    gatsby-plugin-google-analytics: 2.0.17 => 2.0.17 
    gatsby-plugin-manifest: 2.0.24 => 2.0.24 
    gatsby-plugin-netlify: 2.0.12 => 2.0.12 
    gatsby-plugin-nprogress: 2.0.10 => 2.0.10 
    gatsby-plugin-offline: 2.0.25 => 2.0.25 
    gatsby-plugin-react-helmet: 3.0.9 => 3.0.9 
    gatsby-plugin-sharp: ^2.0.28 => 2.0.28 
    gatsby-plugin-sitemap: 2.0.9 => 2.0.9 
    gatsby-plugin-styletron: 3.0.5 => 3.0.5 
    gatsby-plugin-typography: 2.2.9 => 2.2.9 
    gatsby-remark-autolink-headers: 2.0.16 => 2.0.16 
    gatsby-remark-copy-linked-files: 2.0.10 => 2.0.10 
    gatsby-remark-emoji: 0.0.2 => 0.0.2 
    gatsby-remark-images: 3.0.9 => 3.0.9 
    gatsby-remark-prismjs: 3.2.5 => 3.2.5 
    gatsby-remark-responsive-iframe: 2.0.10 => 2.0.10 
    gatsby-remark-smartypants: 2.0.9 => 2.0.9 
    gatsby-source-filesystem: 2.0.24 => 2.0.24 
    gatsby-transformer-remark: 2.3.2 => 2.3.2 

Summary

When headers contain null gatsby-plugin-typography fails to process headers when moving typography related header components to the top.

I can create a PR with a simple null check fix if that's ok, unless this is the correct behaviour.

[pieh]

React.ReactNode[], which can include null as valid values. In my project one of the other plugins injects a null into this array

While this is is valid ReactNode - we probably should filter out any null values from components array in Gatsby I think:

In

gatsby/packages/gatsby/cache-dir/static-entry.js

Lines 71 to 111 in 95155e0

	const setHeadComponents = components => {
	headComponents = headComponents.concat(components)
	}
	const setHtmlAttributes = attributes => {
	htmlAttributes = merge(htmlAttributes, attributes)
	}
	const setBodyAttributes = attributes => {
	bodyAttributes = merge(bodyAttributes, attributes)
	}
	const setPreBodyComponents = components => {
	preBodyComponents = preBodyComponents.concat(components)
	}
	const setPostBodyComponents = components => {
	postBodyComponents = postBodyComponents.concat(components)
	}
	const setBodyProps = props => {
	bodyProps = merge({}, bodyProps, props)
	}
	const getHeadComponents = () => headComponents
	const replaceHeadComponents = components => {
	headComponents = components
	}
	const getPreBodyComponents = () => preBodyComponents
	const replacePreBodyComponents = components => {
	preBodyComponents = components
	}
	const getPostBodyComponents = () => postBodyComponents
	const replacePostBodyComponents = components => {
	postBodyComponents = components
	}

We could add utility

const removeNulls = components => {
  if (Array.isArray(components)) {
    // remove falsy items
    return components.filter(val => val)
  } else {
    // we also accept single components, so we need to handle this case as well
    return components ? [components] : []
  }
}

And then adjust setXComponents:

  const setHeadComponents = components => {
-   headComponents = headComponents.concat(components)
+   headComponents = headComponents.concat(removeNulls(components))
  }

and replaceXComponets

  const replacePostBodyComponents = components => {
-   postBodyComponents = components
+   postBodyComponents = removeNulls(components)
  }

I can create a PR with a simple null check fix if that's ok, unless this is the correct behaviour

That would be great too. As mentioned above this list ideally is sanitized already, but some sanity checks won't hurt

[danielkov]

I've approached it from the plugin side - as it targets this particular issue - and added fault tolerancy and tests.