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 createLayout api + multiple layouts + layout queries #1503

Merged
merged 43 commits into from
Aug 8, 2017

Conversation

craigmulligan
Copy link

@craigmulligan craigmulligan commented Jul 14, 2017

connects to #1494 && #1255
After discussions with @jbolda, we thought it'd be a good idea to add layouts to the redux store and give em their own api.

This PR adds:

  • layouts to redux store.
  • createLayout, deleteLayout actions
  • createLayouts onCreateLayout node api hooks
  • component-layout-creator internal plugin to auto create all layouts in /src/layouts.
  • Ability to add graphql queries to layouts

It doesn't add:

  • Nested layouts

Behaviour:
If no layouts in src/layouts and pages don't have a page.layout attribute then no layout will be loaded.
If src/layouts/index.js exists then it will be loaded for all pages that don't have a page.layout attribute.
If you create a layout with createLayout and you assign it when creating a page(see example) then it'll load the specified layout.

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Jul 14, 2017

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jul 14, 2017

Deploy preview failed.

Built with commit f520828

https://app.netlify.com/sites/gatsbyjs/deploys/597b1460cf321c5521a3a4c7

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jul 14, 2017

Deploy preview ready!

Built with commit 08bd0f3

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

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jul 14, 2017

Deploy preview ready!

Built with commit 08bd0f3

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

@KyleAMathews
Copy link
Contributor

This looks great — this is definitely the next step to get working.

I haven't read through the code in detail but I like the idea of copying the page patterns for layouts. I hadn't really thought about manually adding layouts but yeah, we definitely should expose that capability for more advanced use cases (like say Landr ;-))

@craigmulligan
Copy link
Author

Okay, but we should change the model slightly? or do you think it's fine to treat them exactly the same?

As I mentioned layout.path doesn't make a lot of sense as an id, but it does make sharing functions between the layouts and pages easier haha.

@@ -65,7 +65,7 @@ program
)
.action(command => {
// Set NODE_ENV to 'production'
process.env.NODE_ENV = `production`
// process.env.NODE_ENV = `production`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this stay commented?

Copy link
Author

Choose a reason for hiding this comment

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

Nope I was just testing something, thanks for pointing it out 👍

@@ -1,10 +1,12 @@
// @flow
import Joi from "joi"
import chalk from "chalk"
const _ = require(`lodash`)
const _ = require(`lodash`
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra return

@KyleAMathews
Copy link
Contributor

We should treat them the same in that they're both first class objects in Gatsby-land but otherwise, they're different. Layouts aren't tied to paths so yeah, that doesn't make any sense.

@@ -18,7 +18,9 @@ window.___emitter = emitter
import pages from "./pages.json"
import ComponentRenderer from "./component-renderer"
import asyncRequires from "./async-requires"
import syncRequires from "./sync-requires"
Copy link
Contributor

Choose a reason for hiding this comment

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

sync-requires is just for development. It creates one giant bundle which is fine for dev but not for prod. asyncRequires has all the layout bundles.

@@ -67,7 +68,7 @@ const fetchResource = (resourceName, cb = () => {}) => {
// Find resource
const resourceFunction =
resourceName.slice(0, 6) === `page-c`
? asyncRequires.components[resourceName]
? asyncRequires.components[resourceName] || asyncRequires.layouts[resourceName]
Copy link
Author

Choose a reason for hiding this comment

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

Would there ever be a clash between page chunkNames and layout chunk names? If so we should name them differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Layout chunks are at async.layouts

Copy link
Contributor

Choose a reason for hiding this comment

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

asyncRequires I meant

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm nvm, on phone so not entirely sure what this code is doing.

Copy link
Contributor

Choose a reason for hiding this comment

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

But in any case, no, the two component names are created differently.

Copy link
Author

Choose a reason for hiding this comment

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

Yea, it's created from their filename so we should be good using the same naming func.

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Jul 16, 2017

Deploy preview failed.

Built with commit 0317f6d

https://app.netlify.com/sites/using-glamor/deploys/5980f6eda700c45c623def12

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Jul 16, 2017

@craigmulligan craigmulligan changed the title [WIP] add createLayout api add createLayout api Jul 16, 2017
@craigmulligan
Copy link
Author

Okay I think it's mostly there. @KyleAMathews could you give it a review?

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.

Won't have computer time until tomorrow at the earliest but left some notes on my phone.

.on(`unlink`, path => {
// Delete the page for the now deleted component.
store.getState().layouts.filter(p => p.component === path).forEach(layout => {
deletePage({ name: layout.name })
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like some copied code needs updated

export const layoutSchema = Joi.object()
.keys({
name: Joi.string().required(),
matchPath: Joi.string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is extra

matchPath: Joi.string(),
component: Joi.string().required(),
componentChunkName: Joi.string().required(),
context: Joi.object(),
Copy link
Contributor

Choose a reason for hiding this comment

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

As is this

* @param {string} page.component The absolute path to the component for this layout
* @example
* createLayout({
* path: `myNewLayout`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Example outdated.

Also, if you're using name as an id, perhaps we should just call it an id.

// TODO: We currently don't support context for layouts but left here
// as will implement soon.
// Ensure the layout has a context object
if (!layout.context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. So you are thinking of supporting context on layouts.

Copy link
Author

Choose a reason for hiding this comment

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

I've removed it for this PR it's easy enough to add in the next PR

...this.props,
...this.state.pageResources.json,
})
return createElement(
Copy link
Author

Choose a reason for hiding this comment

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

@KyleAMathews can you sanity check here, I've moved the layout rendering to the component-renderer, which makes things neater, but I want to make sure I didn't overlook anything when making the move.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it cleaner to put layouts in here? I explicitly pulled page rendering into here to separate it from layout rendering as there's some tricky logic around when/how to render pages. What's your reasoning?

Copy link
Author

Choose a reason for hiding this comment

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

so layouts can now change on route changes, so they need to be handled within the pageResources state, I we could create another stateful component but IMO this cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, we can roll with this for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

This stuff needs refactored anyways so... :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work as it turns out because it means layouts are re-rendered on every page change which breaks the contract that they're stateful which breaks among other things gatsbygram.

Also it's less performant.

Thinking about alternatives...

@craigmulligan
Copy link
Author

@KyleAMathews hold off on reviewing for a little while, I'm close to finishing off changes for supporting queries in layouts so I think we can just merge the two PRs into one since they touch on mostly the same code.

@craigmulligan
Copy link
Author

I've updated this PR to include layout queries, as well as refactoring some of the redux model, everything except query watching seems to be working. I'll be back tomorrow to try finish it off, but if anyone wants to review in the meantime 🥇.

)
let layout
if (syncRequires.layouts[page.layoutComponentChunkName]) {
layout = syncRequires.layouts[page.layoutComponentChunkName]
Copy link
Author

Choose a reason for hiding this comment

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

Note to self static entry isn't loading json

action.payload.componentPath = normalize(action.payload.componentPath)
// console.log(state)
Copy link
Author

Choose a reason for hiding this comment

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

rm

@@ -55,6 +55,7 @@ const saveState = _.debounce(state => {

store.subscribe(() => {
const lastAction = store.getState().lastAction
// console.log(lastAction.type)
Copy link
Author

Choose a reason for hiding this comment

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

rm

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Jul 19, 2017

Deploy preview failed.

Built with commit f520828

https://app.netlify.com/sites/using-remark/deploys/597b1461cf321c5521a3a4e5

exports.onCreateLayout = true

/**
* Called when a new layout is created. This extension API is useful
Copy link
Author

Choose a reason for hiding this comment

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

Fix docs

@craigmulligan craigmulligan changed the title add createLayout api add createLayout api + multiple layouts + layout queries Jul 24, 2017
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.

Ugh, made a bunch of review comments on my phone a few days ago that didn't get submitted.

/**
* Create a layout.
* @param {Object} layout a layout object
* @param {string} page.id Unique id for layout
Copy link
Contributor

Choose a reason for hiding this comment

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

little thing but this should be layout.id not page.id

@@ -0,0 +1,44 @@
import React from "react"
Copy link
Contributor

Choose a reason for hiding this comment

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

html.js isn't required anymore. I think generally people shouldn't have their own html.js and just use the default as it's just more code to understand. Instead people should use plugins and things like gatsby-plugin-react-helmet to manage stuff in their <head>. So if this is just a standard html.js file, please remove it.

exports.onCreateLayout = ({ layout, boundActionCreators }) => {
// Note you may see this logged more times than you'd expect
// that's because gatsby automatically creates a layout for each file in /src/layouts/
console.log('Whooop! A new layout was created!')
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps log out the layout.id as well

@@ -0,0 +1,23 @@
const path = require('path')
// Example if you need to programmatically create a layout
Copy link
Contributor

Choose a reason for hiding this comment

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

add "i.e. the layout component is not in src/layouts/"

@@ -57,6 +58,15 @@ actions.createPage = (page, plugin = ``, traceId) => {
internalComponentName = `ComponentIndex`
}

// if no layout is set we check if `/src/layouts/index`
Copy link
Contributor

Choose a reason for hiding this comment

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

Make comments sentences so capitalize the first letter + end with a period.

Also "if it" on second line is extra.

...this.props,
...this.state.pageResources.json,
})
return createElement(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it cleaner to put layouts in here? I explicitly pulled page rendering into here to separate it from layout rendering as there's some tricky logic around when/how to render pages. What's your reasoning?

@@ -19,13 +19,14 @@ const invariant = require(`invariant`)
const normalize = require(`normalize-path`)

exports.extractQueries = () => {
const pages = store.getState().pages
const components = _.uniq(pages.map(p => p.component))
// TODO We can just grab the components straight from store here?
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 yup, the store is always up-to-date.

* @example
* createLayout({
* id: `myNewLayout`,
* component: path.resolve('./src/templates/myNewLayout.js`)
Copy link
Contributor

Choose a reason for hiding this comment

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

mis-matched quote types. Just use a back tick since the default in Gatsby.

@@ -48,7 +49,7 @@ const pascalCase = _.flow(_.camelCase, _.upperFirst)
* })
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't let me comment above, but the following seen above doesn't have matching quotes:
component: path.resolve('./src/templates/my-sweet-new-page.js`),

@KyleAMathews
Copy link
Contributor

Hey Craig, I started working on this locally earlier. Almost got it ready to land. Main missing thing is hot reloading for layout queries. Willh changes in morning and merge. Thanks for all your hard work! This is a huge leap forward.

@jbolda
Copy link
Contributor

jbolda commented Aug 8, 2017

@craig-mulligan I figured it out. I thought it might be a misunderstanding on my end, hah. It would be nice to compose / nest layouts, but this is great even as is.

I have using-javascript-transforms updated and working with this 💯 . @KyleAMathews, I am assuming that it makes sense just to do a separate PR after this lands?

@craigmulligan
Copy link
Author

Awesome!! I'm also really amped excited for nested layouts too. Happy to lend a hand in the PRs to follow :)

@Tom-Bonnike
Copy link
Contributor

Thanks for this PR. I needed the ability to add graphQL queries in layouts to migrate a project to V1. :)

@KyleAMathews
Copy link
Contributor

@craig-mulligan me too ❤️ there's a ton of use cases for nested layouts so excited for it.

* When creating layouts, only pass in the layout path and then
automatically create the id from the name of the component file. This is
safe as unless someone is doing something unusual layouts should all be
in the src/layouts directory. If someone is manually creating layouts,
they'll need to be careful to not overwrite layouts created elsewhere.
* Fix hot re-running of queries in layouts & pages.
@KyleAMathews
Copy link
Contributor

Ok GatsbyGram is back with latest commit https://598a253c7960b15847092350--gatsbygram.netlify.com/

@KyleAMathews
Copy link
Contributor

Ok I think this is ready to go. I really dislike how complicated this code is. Every time I touch it it takes forever to load it into my head which suggests we're using the wrong abstractions or something. Haven't figured out better way yet. Someone please come along and PR a much better implementation :-P

A bit of context for this commit 750d273 Gatsby loads code + data seperately for pages as it's quite often to have a page component power many pages e.g. a blog post component. But since layouts can only have one instance, I changed things so that the code + data are bundled together to avoid making two requests where one would do.

@KyleAMathews
Copy link
Contributor

And for the inevitable breakage this will cause, sorry 🙏

We definitely need better abstractions / tests for these parts of the code.

@KyleAMathews KyleAMathews merged commit a38f49e into gatsbyjs:master Aug 8, 2017
@@ -125,9 +125,16 @@ actions.deleteLayout = (layout, plugin = ``) => {
*/
actions.createLayout = (layout, plugin = ``, traceId) => {
layout.id = path.parse(layout.component).name
layout.componentWrapperPath = path.join(
Copy link
Contributor

Choose a reason for hiding this comment

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

path.join should be joinPath as path.join breaks in windows.

if (!fs.existsSync(dest)) {
fs.writeFile(dest, `{}`, () => {})
}
const jsonDest = path.join(
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be joinPath, but does it make sense to require('../../utils/path')? Feels odd traversing packages, but that does fix it :)

@aeneasr
Copy link

aeneasr commented Sep 12, 2017

Are there any updates on nested layouts? If not, I could take a shot. I'm a capable developer, but I would need some hand-holding in identifying the right components to modify.

@KyleAMathews
Copy link
Contributor

@jbolda has an open PR for this that I'd be he'd love some eyes on #1895

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.

8 participants