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

Improve loading pattern for tags and navItems #461

Merged
merged 5 commits into from
Dec 17, 2018

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Dec 14, 2018

Resolves #458
Impact: minor
Type: performance

Issue

For a shop with >200 tags, paging would be necessary when loading tags to build the navigation tree, and this was causing slow loading of the top nav and/or errors.

Solution

The way we were doing the loading was susceptible to extra looping due to calling fetchMore within a render function. However, Apollo's Query component does not give any better ways to do it. There is an onCompleted function, but it seems to get called only sporadically (maybe only when loading from network and not from cache).

Furthermore, our aim with loading all of the tags is actually to build a single tree for nav, so it's beneficial if we can loop through and load all of the pages, and then build the nav, prior to loading the app at all.

So for now I am doing all of the loading and building in the getInitialProps of the _app.js layout. I'm then passing those to a MobX Provider, which components that need tags or navItems are now injecting.

This seems to work much better. It should be loading from the Apollo cache after the first time, so it should be good performance as far as API and database hits.

We will soon rewrite this to load the entire nav tree at once, using the new defaultNavigationTree query, but that isn't released yet. At that time, we may be able to revert back to using a Query component since it will then be a single query rather than looping pages.

Breaking changes

If you have custom components using tags or navItems, you'll need to inject those props from MobX and the tag structure has been flattened a bit.

Testing

  1. Load approx 250 tags into the database for your primary shop. Make sure they are all visible and not deleted, and either top-level or listed as a subTag for another tag.
  2. Test storefront home page, tag page, and product detail page.
    • Verify that the top navigation looks as expected and works correctly on all pages
    • Verify that the breadcrumbs on all pages look right and work correctly.

Nav loading should be relatively fast and without errors.

@aldeed aldeed self-assigned this Dec 14, 2018
@aldeed aldeed added this to the 🏔 Shavano milestone Dec 14, 2018
@aldeed aldeed requested a review from Akarshit December 14, 2018 17:52
@aldeed
Copy link
Contributor Author

aldeed commented Dec 14, 2018

@Akarshit Unfortunately I could not find any easy fix other than a significant rewrite of how tags and nav are loaded. If you are able to pull this in to the fork fixing conflicts, it would be good to verify that this fixes your issue.

Copy link
Member

@kieckhafer kieckhafer 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 to me. Storefront loaded very quickly with 250 tags, in the navigation. Breadcrumbs worked as they should. I'll leave this up to @Akarshit to merge if he sees the improvement.

@Akarshit
Copy link
Contributor

Yes it works. The tags load up instantaneously.

@Akarshit Akarshit merged commit bef8413 into develop Dec 17, 2018
@aldeed aldeed deleted the fix-458-aldeed-tag-loading-issues branch December 17, 2018 21:55
This was referenced Jan 15, 2019
@spencern spencern mentioned this pull request Jan 25, 2019
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.

3 participants