Config on frontloadServerRender to allow nested frontload components

[davnicwil]

• new options arg in frontloadServerRender with maxNestedFrontloadComponents option, allows setting the max levels of nesting which will be rendered on the server
• new tests for this functionality
• an example of usage added to the example application
• documentation in the README (read the new docs for much more info on this feature)
• some general tidying of unrelated things also included in this PR

[matthewlein]

Sweet!

I like that you are using a promise queue instead of comparing HTML.

Error message on incomplete frontloads is a great idea.

One thing that I noticed when I was digging around was that the withLogging flag is undocumented, which seems super useful, especially now that you might be dealing with unknown server rendering issues.

Agreed that it looks like a great first start.

(I decided to adapt some APIs to accept url slugs to avoid this and the penalty of serial requests, but I know it will come up again)

[davnicwil]

Yeah I was quite pleased to find that promise queue size trick rather than the render output comparing, it's obviously a lot faster / less brittle and as an additional side benefit will work with stream rendering too - something there's already a PR for and I'm intending to add next.

On the withLogging flag - that's a good point. I'm sure there was a reason I chose to not document that that I'm not thinking of, but I'll review that and will probably end up adding it to the docs.

[GytisT]

Hey @davnicwil , what is missing to get this pull request through? Any chance to use nested components? There’s no simple way to deal with situation where react router is used and two frontloading components are required. One, where react-router main switch is placed (body component in my case) to load up any common data shared between the pages and one frontload component for actual page rendered by react-router

[davnicwil]

Hey @GytisT -- this PR will be ready to go once I've merged in the bugfix in #28 which was shipped in v1.0.6

Should be able to get to this this weekend 👍

[matthewlein]

@GytisT If your data is used everywhere for every render, you could maybe do it server-side first and then do your frontload render after it's finished. In my case that was a good way to go, and because I am using redux, I can put the result in the initial state for the store.

fetch(...).then(data => {
  ...
  frontloadServerRender()
    .then(/* render */)
  ...
})

[davnicwil]

@GytisT @matthewlein I've been looking into this this afternoon, and it's turned out to be more complex than I originally thought to merge these two concepts together.

I want to take some more time with this and really be sure that the approach is sound and I'm not going to introduce any more weird async bugs. With the nested frontloading there's a lot more async stuff happening and potential for race conditions, so it's especially important.

@GytisT it shouldn't be too much longer, but I hope your local hacked together solution(s) suffice for a bit longer until this lands! I think @matthewlein's suggestion above is also a good workaround to support nesting.

[davnicwil]

@GytisT @matthewlein OK, this PR is ready to go now.

In doing it I discovered a much simpler way to fix the parallel server render issue that 1.0.4-1.0.6 were attempting to fix, without having to change either the API or care about node version (it works on all versions) - this means that all that stuff about manual context injection, or any context injection at all, has gone away.

It's actually similar to the initial approach - it turns out the approach in general of using a global queue for frontload 'collection' is, I think, correct as long as we absolutely guarantee that the global queue is cleared synchronously after each render - that was the part that wasn't always true before, leading to multiple parallel renders sometimes interacting with the same queue.

So this PR adds that bugfix, and adds the nested frontloads feature on top. I'm going to do a bit more manual testing etc before releasing, but @GytisT if you could clone this branch and test it with your app in the meantime, and report any issues if any come up, that would be much appreciated.

[GytisT]

@davnicwil thanks for your work, I will let you know how it goes with the update in under 24 hours.

[davnicwil]

OK I've tested and for me it looks good, going to ship this