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: GraphQL query & resolver for loading the primary shop #4747

Merged
merged 6 commits into from
Oct 18, 2018

Conversation

dancastellon
Copy link
Contributor

Resolves #4727
Impact: minor
Type: feature

Solution

This PR adds a primaryShop GraphQL query & resolver, eliminating the need to first query for the primary shop ID, followed by another query for shop by ID.

Breaking changes

None

Testing

  1. Open Graphiql and run this query:
{
  primaryShop {
    _id
    name
  }
}

@dancastellon dancastellon requested a review from aldeed October 16, 2018 21:26
@dancastellon dancastellon changed the base branch from master to release-2.0.0-rc.5 October 16, 2018 21:26
aldeed
aldeed previously requested changes Oct 17, 2018
export default async function primaryShopId(_, __, context) {
const shopId = await context.queries.primaryShopId(context.collections);
const shop = await context.queries.shopById(context, shopId);
return shop;
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 nice and simple, but it does two Shops.findOne calls when we often could do just one. You can make a separate primaryShop query function that does:

const domain = url.parse(context.rootUrl).hostname;
let shop = await Shops.findOne({ domains: domain });
if (!shop) {
  shop = await Shops.findOne({ shopType: "primary" });
}
return shop;

And then call that in this resolver

* @param {Object} parentObject - unused
* @param {Object} args - unused
* @param {Object} context - an object containing the per-request state
* @return {Promise<o>} The shop, based on the domain in ROOT_URL
Copy link
Contributor

Choose a reason for hiding this comment

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

Should say Promise<Object>

* @param {Object} context - an object containing the per-request state
* @return {Promise<o>} The shop, based on the domain in ROOT_URL
*/
export default async function primaryShopId(_, __, context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Function name should be primaryShop

@dancastellon dancastellon requested a review from aldeed October 17, 2018 17:02
@aldeed aldeed changed the base branch from release-2.0.0-rc.5 to release-2.0.0-rc.6 October 18, 2018 21:31
@aldeed aldeed merged commit 947bbb8 into release-2.0.0-rc.6 Oct 18, 2018
@aldeed aldeed deleted the feat-4727-dancastellon-primaryShop-query branch October 18, 2018 21:32
@spencern spencern mentioned this pull request Oct 20, 2018
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