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

Add new onPreRenderHTML SSR API. #6760

Merged
merged 13 commits into from
Aug 16, 2018

Conversation

octalmage
Copy link
Contributor

@octalmage octalmage commented Jul 25, 2018

This API allows you to modify/replace the headComponents array before they get passed to html.js. The need for this new API came up in #6302 where CSS added to a site gets inlined before the CSS added by plugins. This causes an issue when using plain CSS with the Typography.js plugin. The CSS for the site gets loaded first, then Typography.js which changes the display of the site.

An example of how to use the onPreRenderHTML API:

exports.onPreRenderHTML = ({ getHeadComponents, replaceHeadComponents }) => {
  const headComponents = getHeadComponents()
  headComponents.sort((x, y) => {
    if (x.key === 'TypographyStyle') {
      return -1
    } else if (y.key === 'TypographyStyle') {
      return 1
    }

    return 0
  })

  replaceHeadComponents(headComponents);
};

This resolves the issue with Typography.js for me, and we might want to follow up by adding it to gatsby-plugin-typography.

I only added two methods, replaceHeadComponents and getHeadComponents since those were the two that I knew we needed. This API can be extended in the future if necessary to modify what gets rendered.

getHeadComponents, replaceHeadComponents, getPostBodyComponents, and replacePostBodyComponents are passed in the initial version of the API.

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Jul 25, 2018

Deploy preview for using-postcss-sass failed.

Built with commit c9228e6

https://app.netlify.com/sites/using-postcss-sass/deploys/5b6b255fc6aed64d95a15016

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jul 25, 2018

Deploy preview for using-drupal ready!

Built with commit c9228e6

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

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jul 25, 2018

Deploy preview for gatsbygram ready!

Built with commit c9228e6

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

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Jul 25, 2018

Deploy preview for gatsbyjs failed.

Built with commit e2b0def

https://app.netlify.com/sites/gatsbyjs/deploys/5b6b1a9f3813f038ca3a564a

@m-allanson
Copy link
Contributor

Thanks @octalmage 👍 This seems like a reasonable approach if we can't automatically order everything correctly all the time. Pinging @KyleAMathews as chief API wrangler to take a look.

@octalmage
Copy link
Contributor Author

Now that I think about it, a snapshot test might be a little heavy handed for this. Any change to the elements added to a page will cause the test to fail. Not sure if this is desired, but there aren't existing tests around this so 🤷‍♂️.

@KyleAMathews
Copy link
Contributor

Looks great! @m-allanson, @pieh, and I discussed this earlier in an maintainers meeting and we agreed that this is the best option for changing the order of components. The other option is letting plugins assign "weights" to their component which could get messy quickly e.g. classic z-index problems with different elements duking it out for top position.

This API could also have other uses e.g. removing components you don't want.

One change, could you add similar support for changing postBodyComponents?

@octalmage
Copy link
Contributor Author

@KyleAMathews added getPostBodyComponents and replacePostBodyComponents!

I added a warning statement similar to replaceRenderer, but I don't think it's 100% accurate since multiple plugins can safely implement this API if done correctly. How about a warning like this:

WARNING This API can easily break your site. Make sure you use getPostBodyComponents to modify the existing components instead of replacing them completely.

Or something like that. The idea being you should be a good plugin citizen and return the full set of components except for in rare circumstances.

@octalmage
Copy link
Contributor Author

Any thoughts on my notes above? I think this is ready to go, we could always follow up if the docs are confusing.

@pieh
Copy link
Contributor

pieh commented Aug 8, 2018

Can we avoid "last plugin wins problem" here? Is this even desirable

@octalmage
Copy link
Contributor Author

Do you mean the wording, or the actual issue with a last plugin winning?

@octalmage
Copy link
Contributor Author

To cover the case of the last plugin winning problem, you do need to be able to modify the output of other plugins here. So if the last plugin in the list just removes all head elements, then there will be no head elements. But if plugins just modify existing elements using getHeadComponents and return the full set, multiple plugins should be able to use this API.

WordPress has a similar problem, and solves this a similar way. For example the the_content filter:

// Good plugin.
add_filter( 'the_content', function( $content ) {
    return str_replace( 'tset', 'test', $content );
} );

// Bad plugin
add_filter( 'the_content', function() {
    return 'replace all content';
} );

I think it's up to us to make sure at least official plugins use the API correctly.

@pieh
Copy link
Contributor

pieh commented Aug 8, 2018

Do you mean the wording, or the actual issue with a last plugin winning?

At first I was thinking about issue, but after rereading PR I think this will allow more than 1 plugin to reorder, given that You added getHeadComponents function and not passed array of components directly.

Also for full parity I think same treatment for setPreBodyComponents (even if it's not really used that much and shouldn't cause issues)?

@octalmage
Copy link
Contributor Author

@pieh done! Thanks!

@octalmage
Copy link
Contributor Author

The Gatsbygram ui test failures seem unrelated.

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

@pieh pieh merged commit c31de0c into gatsbyjs:master Aug 16, 2018
@gatsbot
Copy link

gatsbot bot commented Aug 16, 2018

Holy buckets, @octalmage — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. You’ll receive an email shortly asking you to confirm. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@octalmage octalmage deleted the topics/replace-head-components branch August 16, 2018 21:41
porfirioribeiro pushed a commit to porfirioribeiro/gatsby that referenced this pull request Aug 22, 2018
* WIP.

* Add onRenderHead API.

* Starting tests.

* More mocking.

* Update snapshots and fix mocks.

* Give null component for page.

* Updated API to use getHeadComponents so the array doesn't get out of sync.

* Add replacePostBodyComponents function.

* Fix test comment.

* Add getPreBodyComponents and replacePreBodyComponents to onPreRenderHTML.

* Should be testing StaticEntry, not DevelopStaticEntry.

* Account for reach-router changes.
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.

5 participants