-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
fix(gatsby): filter null values in headComponents, preBodyComponents and postBodyComponents #12555
Conversation
onPreRenderHTML: ({ getHeadComponents }) => { | ||
const headComponents = getHeadComponents() | ||
expect(headComponents.includes(null)).toBeFalsy() | ||
expect(headComponents.find(val => Array.isArray(val) && val.length === 0)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
It would be great to apply that to |
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 { |
There was a problem hiding this comment.
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
There was a problem hiding this 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 👍
Description
Related Issues
Fixes #12553