Docs SSR

[iAdramelk]

• Make docs page indexable by search engines.
• Add proper 404 errors for non-existing pages (partial fix for server: better way to handle HTTP redirects  #757).
• Fixes sidebar blinks on next/prev buttons click #642.

UPDATE: See longer explanation in #834 (comment).

TODOS:
- [ ] Add smooth scroll for anchors
- [ ] Add preloaders on page change (not sure if we want them now)

[shcheklein]

@iAdramelk I think we are fine w/o either of those for now. In case of network failure it will show an error, right?

if smooth scroll is not something hard to do - let's do this

please, resolve the conflicts - merge/rebase

[iAdramelk]

@shcheklein

Smooth scrolling should have been easy, but wasn't for some reason. System method to do it using window.hashchange didn't work because Next Router considering it as navigation event and adds it's logic to it. And Next Router's special event for it hashChangeComplete for some reason didn't fire. Tried to debug it yesterday for couple of hours, but was not successful.

Will text for network failure, but it should work.

[shcheklein]

but wasn't for some reason.

yep, sounds like a strange problem to have ... let's not waste time on this - I think it's totally ok w/o smooth anchors

if it works fine across different browsers let's merge

[jorgeorpinel]

I'd just return a 504 here. Or some other generic status code.

[jorgeorpinel]

Maybe with an error message including the res.status.

[iAdramelk]

Why not leave actual code there? It can help us debug things. Do you see some case there showing real code can be bad?

[jorgeorpinel]

Ah, this fetch is for the internal document? Not some sort of gateway? In that case I think returning actual code makes sense indeed. Although a generic code with a message explaining what happened (and the source error code) would make ever more sense I think. This is merged and works though, so whatevs.

[jorgeorpinel]

Hmmm. what kind of errors can fetch throw that would get caught here? Can't easily tell from https://github.com/matthew-andrews/isomorphic-fetch

[iAdramelk]

fetch is a promise. It can return either Promise.resolve or Promise.reject. Resolve is the thing that we will receive in the most cases. It have response object and we can check it's status field. But if, for example, your internet connection is lost, the browser will not resolve it, but will instead throw error which we are catching there.

Try to set connection to offline in Network tab of Developers Tools and then try to click on the internal link, it will throw error.

[jorgeorpinel]

Hmmm OK but why 404? I see this is changed though... Will re-ask.

UPDATE: See #834 (review)

[jorgeorpinel]

Make docs page indexable by search engines.

Sorry, can you briefly explained how this is achieved by this PR? Sounds great but I'm not easily getting it from browsing through the file changes and clicking around the nav inside https://dvc-org-pr-834.herokuapp.com/doc seems completely dynamic to me.

[shcheklein]

Left a few comments/questions. PTAL.

[iAdramelk]

@jorgeorpinel

Sorry, can you briefly explained how this is achieved by this PR? Sounds great but I'm not easily getting it from browsing through the file changes and clicking around the nav inside https://dvc-org-pr-834.herokuapp.com/doc seems completely dynamic to me.

Basically how it worked before:

1. All /docs URLs were redirected to pages/doc.js.
2. doc.js was rendered on server with blank content (because componentDidMount didn't fire on the server) and was sent to browser this way.
3. In browser, after page was loaded, componentDidMount did fire. It was checking current url, and fetching the corresponding md-file from the server then.
4. After that, we were adding a listener to the window.location changes and were implementing navigation by hand.

What changed:

1. componentDidMount was replaced with getInitialProps. Because Next.js can run getInitialProps on the server, it can now populate content zone and menu with actual data before sending the page to the client (try to disable JavaScript in the browser and load some page from docs).
2. Our custom navigation and routing logic was replaced with already existing Next.js components Link and Router. They allow us to navigate between pages on the client side without the need to download everything from the server again (like our custom logic did before). Basically, they change window.location, and then fire getInitialProps if needed. They also give us hooks to attach to page transition events and errors if we need to.
3. Also, I implemented some checks for docs pages in getInitialProps, and if md-file didn't exist, it returns standard Next.js error page as all other routes except /docs are doing now.

[shcheklein]

@iAdramelk LGTM! Anything else to be fixed or ready to merge? Have you checked it on different devices/browsers?

[iAdramelk]

Fixed error on mobile. Tested in Edge. We can merge it.

[jorgeorpinel]

Thanks for the explanation Alex. Very interesting. And sorry took me a long time to get back to this haha. I see it's merged. I may have a couple questions left...

[jorgeorpinel]

Can't this go into an infinite loop? (Blocked by the browser probably.)

Continuation of #834 (review)

[iAdramelk]

If page loads again then yes, but this error usually means that the internet is down and we will just show browser error page there. Maybe there are cases then this error is thrown in other situations, but I don't know about them.