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

fix(gatsby): filter null values in headComponents, preBodyComponents and postBodyComponents #12555

Merged
merged 2 commits into from
Mar 19, 2019
Merged

Conversation

danielkov
Copy link
Contributor

Description

This is related to #12549 and as discussed in the comments, sanitizing headComponents would ensure that plugins that do similar transformations as the one in that issue can assume that all elements are actual components.
-- #12553

Related Issues

Fixes #12553

@danielkov danielkov requested a review from a team as a code owner March 13, 2019 20:12
onPreRenderHTML: ({ getHeadComponents }) => {
const headComponents = getHeadComponents()
expect(headComponents.includes(null)).toBeFalsy()
expect(headComponents.find(val => Array.isArray(val) && val.length === 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to miss assertion? the expect() should be falsy here as well, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! It's a midnight-coding error on my part.

global.plugins = [setNullHeaderPlugin, checkNonEmptyHeadersPlugin]

StaticEntry(`/about/`, (_, html) => {
expect(html).toMatchSnapshot()
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need snapshot testing here if we assert head components directly? I would like avoid busy snapshots like that that, especially that I think this snapshot would pass right now without any changes in static-entry file, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it stands they do seem to be redundant. I will remove them.

@pieh
Copy link
Contributor

pieh commented Mar 14, 2019

It would be great to apply that to PreBodyComponents and PostBodyComponents as well (would be nice to structure test to not repeat mocked plugins / tests for each of those

@danielkov
Copy link
Contributor Author

It would be great to apply that to PreBodyComponents and PostBodyComponents as well

I will do that. I've scanned the official plugins and none of them seems to rely on the current behaviour. However I will browse through some of the popular plugins outside of this repo, to make sure it doesn't regress community packages.

if (Array.isArray(components)) {
// remove falsy items
return components.filter(val => (Array.isArray(val) ? val.length > 0 : val))
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can drop this else because of the return

@wardpeet wardpeet added the status: awaiting author response Additional information has been requested from the author label Mar 19, 2019
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let's get this in! Thank you @danielkov 👍

@sidharthachatterjee sidharthachatterjee changed the title fix(gatsby) head components check for empty values fix(gatsby): filter null values in headComponents, preBodyComponents and postBodyComponents Mar 19, 2019
@sidharthachatterjee sidharthachatterjee merged commit f7dbc8b into gatsbyjs:master Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting author response Additional information has been requested from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants