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 uniform wrapPageElement and wrapRootElement apis to SSR and Browser #7204

Merged
merged 38 commits into from
Aug 16, 2018

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Aug 10, 2018

We are missing proper hooks and method to wrap either Page components (to restore layout, get some transition solution using plugins) or Router/Root components (to setup Providers) that can be used in both SSR and Browser envs.

Because of that other APIs were missused and their restrictions often caused problems (like limitation to single use of API).

This adds:

  • wrapRootElement to both gatsby-ssr and gatsby-browser
  • wrapPageElement to both gatsby-ssr and gatsby-browser
  • wrapRootElement and wrapPageElement hooks are chainable - meaning more than 1 plugin can use them.

wrapRootComponent is getting removed in favour of wrapPageElement - It also slightly modifies signature of wrapRootComponent, which now should be simpler - return componets and not function that returns components. Also because it is now chainable - Root react component is no longer passed - now component Element is passed which is instance of react component - this was done mainly because I wanted to keep signatures of new APIs as close as possible and this way we won't have to pass page props to components we know nothing about in wrapPageComponent - it's enough that we use component instance that is already created.

Because of this change I added some checks in development - there is still TODO to add this change to migration guide, generate short link and link to it.

This is first part of resolution for #6127

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Aug 10, 2018

Deploy preview for using-postcss-sass failed.

Built with commit e50552b

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

@gatsbybot
Copy link
Collaborator

gatsbybot commented Aug 10, 2018

Deploy preview for gatsbygram ready!

Built with commit e50552b

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

@gatsbybot
Copy link
Collaborator

gatsbybot commented Aug 10, 2018

Deploy preview for using-drupal ready!

Built with commit e50552b

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

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Aug 10, 2018

Deploy preview for gatsbyjs failed.

Built with commit e50552b

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

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Aug 10, 2018

Deploy preview for using-styled-components failed.

Built with commit b1625e2

https://app.netlify.com/sites/using-styled-components/deploys/5b74c472fdd72a20a5f6639c

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Aug 10, 2018

Deploy preview for using-contentful failed.

Built with commit b1625e2

https://app.netlify.com/sites/using-contentful/deploys/5b74c473fdd72a20a5f6639f

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Aug 10, 2018

Deploy preview for using-glamor failed.

Built with commit b1625e2

https://app.netlify.com/sites/using-glamor/deploys/5b74c471fdd72a20a5f66399

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Aug 10, 2018

Deploy preview for using-jss failed.

Built with commit b1625e2

https://app.netlify.com/sites/using-jss/deploys/5b74c475fdd72a20a5f663b7

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Aug 10, 2018

Deploy preview for image-processing failed.

Built with commit b1625e2

https://app.netlify.com/sites/image-processing/deploys/5b74c471fdd72a20a5f66396

@pieh pieh changed the title add uniform wrapPageComponent and wrapRootComponent apis to SSR and Browser add uniform wrapPageElement and wrapRootElement apis to SSR and Browser Aug 15, 2018
},
wrapRootComponent: {
replacement: `wrapRootElement`,
migrationLink: `https://next.gatsbyjs.org/docs/migrating-from-v1-to-v2/#browser-api-wraprootcomponent-was-removed`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: setup short link once this is merged

KyleAMathews
KyleAMathews previously approved these changes Aug 16, 2018
Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

Looks fantastic! Only found a few copy nits that need corrected. Super excited for this to land!

@@ -874,6 +895,26 @@ Import graphql types from `gatsby/graphql` to prevent `Schema must contain uniqu
+const { GraphQLString } = require(`gatsby/graphql`)
```

### Browser API `wrapRootComponent` was removed
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe say, "was replaced with wrapRootElement"?


Wrap pages in component that won't get remounted on page changes.

Wrapping component will have access to all props available in Page components. This is escape hatch to get basic v1 layouts back.
Copy link
Contributor

@KyleAMathews KyleAMathews Aug 16, 2018

Choose a reason for hiding this comment

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

Missing an "an" — "This is a escape"

)
```

And then You can use it anywhere:
Copy link
Contributor

Choose a reason for hiding this comment

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

lower case y in you.

@pieh
Copy link
Contributor Author

pieh commented Aug 16, 2018

Add mention in layout removal section in migration guide pointing to gatsby-plugin-layout

@chasemccoy
Copy link
Contributor

Glad to see this wrapping up, I've been waiting to get this into my site! Thanks for the hard work!

Copy link
Contributor

@m-allanson m-allanson left a comment

Choose a reason for hiding this comment

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

🕺

@m-allanson m-allanson merged commit 8029c66 into gatsbyjs:master Aug 16, 2018
@pieh pieh mentioned this pull request Aug 17, 2018
porfirioribeiro pushed a commit to porfirioribeiro/gatsby that referenced this pull request Aug 22, 2018
…er (gatsbyjs#7204)

* Remove not used API hook

* save browser plugin names in development to identify plugins

* update using-redux example

* allow ssr and browser api runners to use return of previous plugin as input for another

* allow chaining of browser wrapRootComponent api

* add chainable wrapRootComponent api to ssr

* api docs corrections

* add wrapPageComponent to both ssr and browser APIs

* update replaceRouterComponent migration section

* add migration guide section on wrapRootComponent signature change

* add example to wrapPageComponent / wrapRootComponent

* add link to migration guide in console error

* update gatsby-plugin-jss

* styletron plugin and example update

* add gatsby-plugin-layout

* update styled-components plugin

* add using-multiple-providers-example

* fix lint

* add some clarification to docs

* gatsby-plugin-layout version sanity check + readme update

* let preferDefault do its jon - thanks porfirioribeiro

* rename wrapXcomponent to wrapXelement

* update migration guide

* migration guide docs update

* add console warning when wrapRootComponent is used with link to migration guide

* [gatsby-plugin-layout] default to `src/layouts/index.js`

* update gatsby-plugin-layout readme with default settings

* [gatsby-plugin-layout] proper version syntax

* use shortlink

* no need for plugin name, in browser requires list - migration check is done in node env now

* typos

* migration guide section rename

* link to gatsby-plugin-layout in layout component removal section

* missed typo
amberleyromo pushed a commit that referenced this pull request Oct 24, 2018
* Part of #7204
Also added in some language from a quick review.

Signed-off-by: moxiegirl <[email protected]>

* Responding to @amberleyromo comment

Signed-off-by: moxiegirl <[email protected]>

* tweaks
jedrichards pushed a commit to jedrichards/gatsby that referenced this pull request Nov 1, 2018
* Part of gatsbyjs#7204
Also added in some language from a quick review.

Signed-off-by: moxiegirl <[email protected]>

* Responding to @amberleyromo comment

Signed-off-by: moxiegirl <[email protected]>

* tweaks
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
* Part of gatsbyjs#7204
Also added in some language from a quick review.

Signed-off-by: moxiegirl <[email protected]>

* Responding to @amberleyromo comment

Signed-off-by: moxiegirl <[email protected]>

* tweaks
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.

6 participants