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.

Sign up for GitHub

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

Jump to bottom

refactor(gatsby): Split static/page queries #13064

Merged
merged 11 commits into from
Apr 17, 2019
Merged

refactor(gatsby): Split static/page queries #13064

merged 11 commits into from
Apr 17, 2019

Conversation

Moocar
Copy link
Contributor

@Moocar Moocar commented Apr 3, 2019 •
edited
Loading

Description

For #13004, we need to run page queries after build-javascript has executed. This PR introduces the ability to split queries into static/page and run them explicitly.

Related Issues

@Moocar Moocar requested review from a team as code owners April 3, 2019 05:34
@Moocar Moocar mentioned this pull request Apr 3, 2019
Closed
30 tasks
@Moocar Moocar changed the title Split static page queries Split static/page queries Apr 3, 2019
@Moocar Moocar changed the title Split static/page queries refactor(gatsby): Split static/page queries Apr 3, 2019
@Moocar Moocar closed this Apr 3, 2019
@Moocar Moocar reopened this Apr 3, 2019
@Moocar
Copy link
Contributor Author

Moocar commented Apr 3, 2019

I'm noticing that page-query-runner.js is becoming a bad name for that file. It's more acting as a one stop shop for anything query related. Except for stuff covered by watcher. I'm tempting to move this to src/query/index.js.

@KyleAMathews
Copy link
Contributor

Makes sense to me

@Moocar Moocar force-pushed the split-static-page-queries branch from 25357f7 to 899a27f Compare April 4, 2019 22:00
@Moocar Moocar force-pushed the split-static-page-queries branch from 899a27f to 15cc77e Compare April 15, 2019 21:08
@Moocar Moocar mentioned this pull request Apr 15, 2019
Merged
@wardpeet
Copy link
Contributor

@Moocar could you rebase

@Moocar Moocar force-pushed the split-static-page-queries branch from 15cc77e to 5cd8687 Compare April 16, 2019 12:27
@Moocar
Copy link
Contributor Author

Moocar commented Apr 16, 2019

@wardpeet Done, thanks!

wardpeet
wardpeet reviewed Apr 16, 2019
View reviewed changes
Copy link
Contributor

@wardpeet wardpeet 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, a few comments though. But these PRs are very easy to read! Great job!

packages/gatsby/src/bootstrap/index.js Outdated Show resolved Hide resolved
packages/gatsby/src/query/page-query-runner.js Outdated Show resolved Hide resolved
packages/gatsby/src/query/page-query-runner.js Outdated Show resolved Hide resolved
wardpeet
wardpeet approved these changes Apr 16, 2019
View reviewed changes
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

LGTM!

packages/gatsby/src/query/page-query-runner.js Outdated Show resolved Hide resolved
packages/gatsby/src/query/page-query-runner.js Outdated Show resolved Hide resolved
wardpeet and others added 2 commits April 16, 2019 08:03
@wardpeet @Moocar
use dot notation for grouped.static
214d747 
Co-Authored-By: Moocar <Anthony.Marcar@gmail.com>
@wardpeet @Moocar
use dot notation for grouped.page
0926899 
Co-Authored-By: Moocar <Anthony.Marcar@gmail.com>
@wardpeet wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Apr 16, 2019
@DSchau
Copy link
Contributor

DSchau commented Apr 16, 2019

The integration test failure seems actually meaningful? May want to revisit that to ensure we're not introducing anything breaking here--may even need to tweak the integration test itself related to changes in this PR!

@Moocar
Copy link
Contributor Author

Moocar commented Apr 16, 2019

@DSchau Yep. It's legit. I'm looking into it.

@Moocar Moocar removed the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Apr 16, 2019
@Moocar
Copy link
Contributor Author

Moocar commented Apr 16, 2019

The failing integration test was caused by not checking if a page exists before creating a query job for it. There's a case where we create a SitePage node for /dev-404-page/, but only create the corresponding Page during gatsby develop.

A more permanent fix would be to not even create the /dev-404-page/ SitePage during gatsby build, but schema depends on it so we'll have to leave it for another day.

@wardpeet you were totally right in your earlier comment about checking for nulls 🎩

@DSchau DSchau added the status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response label Apr 16, 2019
@KyleAMathews KyleAMathews mentioned this pull request Apr 16, 2019
Closed
KyleAMathews
KyleAMathews approved these changes Apr 17, 2019
View reviewed changes
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.

💯

packages/gatsby/src/query/page-query-runner.js Outdated
}

const processQueries = async (queryJobs, activity) => {
const queue = queryQueue.makeBuild()
Copy link
Contributor

Choose a reason for hiding this comment

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

create is our operator of choice https://www.gatsbyjs.org/docs/api-specification/#operators

So maybe createBuildQueue?

packages/gatsby/src/query/page-query-runner.js Outdated
await queryQueue.processBatch(queue, queryJobs)
}

const makeStaticQueryJob = (state, queryId) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

createStaticQueryJob

KyleAMathews
KyleAMathews approved these changes Apr 17, 2019
View reviewed changes
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.

💥

@KyleAMathews KyleAMathews added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Apr 17, 2019
@Moocar Moocar merged commit cbfffee into gatsbyjs:master Apr 17, 2019
@Moocar Moocar deleted the split-static-page-queries branch April 17, 2019 00:32
@Moocar
Copy link
Contributor Author

Moocar commented Apr 17, 2019

Published in gatsby@2.3.24

@pvdz pvdz mentioned this pull request Nov 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wardpeet wardpeet wardpeet approved these changes

@KyleAMathews KyleAMathews KyleAMathews approved these changes

Assignees
No one assigned
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Moocar @KyleAMathews @wardpeet @DSchau