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

refactor(gatsby): Make query queue constructable #13061

Merged
merged 5 commits into from
Apr 10, 2019

Conversation

Moocar
Copy link
Contributor

@Moocar Moocar commented Apr 3, 2019

Description

#13004 requires that page queries be executed after the javascript build completes. To get there, we need to be able to process a batch of queries. The current code relies on a global query queue, and uses complex global events to ensure it's paused/resumed during bootstrap. This PR makes the queue constructable so that a batch of queries can be processed easily.

I also noticed that develop queue is much more complex that the build queue, so I've separated those into two queue constructors.

Related Issues

@Moocar Moocar force-pushed the query-queue-make-contructable branch from f83d752 to 1b5f5f5 Compare April 4, 2019 21:32
@Moocar Moocar marked this pull request as ready for review April 4, 2019 21:33
@Moocar Moocar requested review from a team as code owners April 4, 2019 21:33
@Moocar
Copy link
Contributor Author

Moocar commented Apr 8, 2019

Conflicts resolved

@DSchau DSchau added status: awaiting author response Additional information has been requested from the author status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response and removed status: awaiting author response Additional information has been requested from the author labels Apr 9, 2019
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, just 2 questions


const queryJobs = makeQueryJobs(pathnamesToRun)

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.

calling it makeBuild is a bit weird here, I get why it's called that way by checking the full PR but it doesn't really fit in my mind 😋 I can't come up with a better name either as I was thinking of makeStatic or makePassive but that's even weirder 😄

Copy link
Contributor Author

@Moocar Moocar Apr 10, 2019

Choose a reason for hiding this comment

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

Yeah, it makes sense within query-queue.js (since there's makeBuild and makeDevelop, but you're right that it's a bit confusing from outside.

The other possibility I considered was to move makeBuild into page-query-runner, and call it makeQueryQueue. That way we could pull makeDevelop directly into src/commands/develop.js (and just call it makeQueryQueue).

Copy link
Contributor

@wardpeet wardpeet Apr 10, 2019

Choose a reason for hiding this comment

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

I like what you're thinking. Moving makeDevelop in src/commands/develop.js probably makes the file more bloated which can also make it harder to understand.

I'll just merge after testing and let's have a look afterwards.

@@ -221,3 +161,72 @@ const findDirtyIds = actions => {
)
return uniqDirties
}

const runInitialQueries = async activity => {
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 sooo much cleaner than what we had before! 👍

@@ -124,7 +124,6 @@ const preferDefault = m => m && m.default || m
writeAndMove(`pages.json`, JSON.stringify(pagesData, null, 4)),
writeAndMove(`sync-requires.js`, syncRequires),
writeAndMove(`async-requires.js`, asyncRequires),
writeAndMove(`match-paths.json`, JSON.stringify(matchPaths, null, 4)),
Copy link
Contributor

Choose a reason for hiding this comment

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

is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I removed it because the match-paths PR wasn't merged when I opened this PR. But that's resolved now. I'll add it back in. Great find!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 76d56ba

@wardpeet wardpeet merged commit 3ca49f2 into gatsbyjs:master Apr 10, 2019
@sidharthachatterjee
Copy link
Contributor

Published in gatsby@2.3.17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants