Load site data asynchronously

[marvinchin]

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Enhancement to an existing feature

Resolves #608.

What is the rationale for this request?

To avoid having render being blocked on waiting for siteData.json to load, we first render the page then load it asynchronously.

What changes did you make? (Give an overview)

• Embed the enableSearch variable in the HTML document
• Update setup.js to fetch siteData.json asynchronously

Is there anything you'd like reviewers to focus on?

I've embedded the enableSearch variable in the HTML document to allow us to decide whether to use setup() or setupWithSearch() without first having to load siteData.json to preserve the original behavior. However, I was wondering whether or not it would be possible for us to simply use setupWithSearch() for both cases, since sites with search disabled cannot use the searchbar component anyway (see #620). This would allow us to avoid having to embed enableSearch in the HTML document.

Testing instructions:
Observe the chrome network tab. siteData.json should be loaded after the page is rendered completely.

[yamgent]

We should be consistent with the slash direction, so let's stick to \.

[acjh]

We should use /, especially for URLs. Is \ generated by Windows here?

[yamgent]

Yes \ is generated by Windows. We currently update test files by copying over content from _site to expected, so the slashes are going to change depending on the OS that the person is using when he/she updates the tests, which is not really ideal...

Maybe code-wise we should standardize how MarkBind output slashes for file paths (can be a separate PR for that).

[acjh]

Agreed.

[marvinchin]

@acjh @yamgent could I check if I should change the slash direction in my PR, or should we leave it to be fixed in another PR?

Thanks for looking into it!

[yamgent]

could I check if I should change the slash direction in my PR, or should we leave it to be fixed in another PR?

Those will be dealt with by the person that is resolving #628.

[marvinchin]

@yamgent I've made the changes!

[yamgent]

However, I was wondering whether or not it would be possible for us to simply use setupWithSearch() for both cases, since sites with search disabled cannot use the searchbar component anyway (see #620). This would allow us to avoid having to embed enableSearch in the HTML document.

I am OK with unifying setup() and setupWithSearch() since the original reason why they were split was mainly because of siteData.json's existence.

However, for your last point, I don't see how this would result in not using enableSearch. If enableSearch is false, then getSearchData() should never be called, so you still need a way to determine the value of enableSearch.

[marvinchin]

@yamgent if we do not use enableSearch, we can simply make the call to fetch siteData.json regardless of whether or not search is enabled. If enableSearch is disabled, then the call will be unnecessary but should not affect the rest of the site.

I suppose this is a tradeoff between making an unnecessary call, and injecting the enableSearch into the document. I was just thinking that injecting the variable into the document felt a little dirty, so I was thinking if there was any way to avoid that 😛

[acjh]

Downloading a file we won't use is bad.

[marvinchin]

If that's the case, perhaps not unifying setup() and setupWithSearch() might still make sense, since that way we do not need to add the searchbar component or the required attributes/methods to the vue instance we are creating if enableSearch is false.

[yamgent]

One minor nit:

[yamgent]

Rename getSearchData() to updateSearchData() (methods with names that start with get are usually accessor methods).

[marvinchin]

@yamgent fixed!

[marvinchin]

@yamgent could I check if we are intending to merge this? Just wanted to let you know that I've resolved the merge conflicts! 🙂