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

feat(gatsby): Move connection out of sift #9508

Merged
merged 11 commits into from
Oct 31, 2018

Conversation

Moocar
Copy link
Contributor

@Moocar Moocar commented Oct 28, 2018

Builds on #9416 . Merge that one before this

In additional to performing the queries, run-sift.js also currently converts the results into connections (if connection query). But as part of #9338, we will be using loki to perform queries too. So we would need to duplicate the connection logic in both query engines.

This PR pulls the connection logic out of run-sift and into build-node-connections which makes sense to me as the query engine doesn't need to know about connections. Only about whether it should return one result or many (thus the firstOnly flag).

Another benefit is that the query engine no longer needs to understand page dependencies.

@Moocar Moocar requested a review from a team as a code owner October 28, 2018 22:51
@pieh
Copy link
Contributor

pieh commented Oct 29, 2018

merged #9416, can you update PR seems like our squashing on merge desynced this branch :(

@Moocar Moocar requested a review from a team October 30, 2018 04:51
@Moocar
Copy link
Contributor Author

Moocar commented Oct 30, 2018

@pieh done

@Moocar Moocar mentioned this pull request Oct 30, 2018
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.

Just minor things. We can skip it them if you feel like it would slow your progress on actual loki things. Just let me know and I'll merge as-is

I love moving createPageDependencies out of run-sift - it makes much more sense to have it in resolver than in run-sift

packages/gatsby/src/schema/build-node-connections.js Outdated Show resolved Hide resolved
packages/gatsby/src/schema/build-node-types.js Outdated Show resolved Hide resolved
pieh and others added 2 commits October 31, 2018 14:02
Co-Authored-By: Moocar <Anthony.Marcar@gmail.com>
Co-Authored-By: Moocar <Anthony.Marcar@gmail.com>
@Moocar
Copy link
Contributor Author

Moocar commented Oct 31, 2018

@pieh I'm definitely a fan of that new "Suggested change" feature :D. Should be good now

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.

Thanks!

@pieh pieh merged commit 8c7a745 into gatsbyjs:master Oct 31, 2018
jedrichards pushed a commit to jedrichards/gatsby that referenced this pull request Nov 1, 2018
**Builds on gatsbyjs#9416 . Merge that one before this**

In additional to performing the queries, `run-sift.js` also currently converts the results into connections (if connection query). But as part of gatsbyjs#9338, we will be using `loki` to perform queries too. So we would need to duplicate the connection logic in both query engines.

This PR pulls the connection logic out of run-sift and into `build-node-connections` which makes sense to me as the query engine doesn't need to know about connections. Only about whether it should return one result or many (thus the `firstOnly` flag).

Another benefit is that the query engine no longer needs to understand page dependencies.
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
**Builds on gatsbyjs#9416 . Merge that one before this**

In additional to performing the queries, `run-sift.js` also currently converts the results into connections (if connection query). But as part of gatsbyjs#9338, we will be using `loki` to perform queries too. So we would need to duplicate the connection logic in both query engines.

This PR pulls the connection logic out of run-sift and into `build-node-connections` which makes sense to me as the query engine doesn't need to know about connections. Only about whether it should return one result or many (thus the `firstOnly` flag).

Another benefit is that the query engine no longer needs to understand page dependencies.
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.

2 participants