Do not fill in inner html on the server-side for a lazy-loaded view

[sarchila]

I've been getting server-side errors when attempting to lazy-load a view that accesses properties on the model/collection in getTemplateData.

The reason is that in the rendr-handlebars view helper, a call is made to get server html https://github.com/rendrjs/rendr-handlebars/blob/master/shared/helpers/view.js#L55, but that call does not take into account whether the view is to be loaded lazily on the client before invoking getInnerHtml server-side.

getInnerHtml attempts to fill in the template data, which in turn accesses model/collection properties on the server when the data hasn't been fetched yet and fails.

[sarchila]

The Travis CI build failure doesn't appear to be related to the code changes and seems to be an existing issue with setting up the environment.

[ingiulio]

@sarchila good catch, and the fix looks fine to me. Would you mind adding a test for it?

[sarchila]

Thanks @ingiulio , look okay?

[ingiulio]

Looks great! Thanks @sarchila

[saponifi3d]

Seems like we should just not be calling getHtml if it's a lazy view, instead we should verify why rendr-handlebars is trying to even render it on the server... i used to have a fix for this problem on my old laptop...

I believe the attachChildViews function is where render is being called incorrectly for lazy views. I was refactoring the callback function from the fetch promise in the attach function and had the correct fix about done before the laptop went away. It should def just not call the getHtml function if it's lazy rather than returning an empty string.

[sarchila]

My understanding was that we'd still want this method to return the wrapper element on the server, just not try to fill in the inner html.

[saponifi3d]

the wrapped element item is created in rendr-handlebars it should just be a placeholder div that is completely replaced on the client after rendering. https://github.com/rendrjs/rendr-handlebars/blob/master/shared/helpers/view.js#L58

[sarchila]

Sort of related, but I'm a bit unclear about the intent of getHtml. Here https://github.com/rendrjs/rendr-handlebars/blob/master/shared/helpers/view.js#L55 in the rendr-handlebars code, the comment makes it seem like view.getHtml should be used on the server to only return the outer wrapper element, but without this change, it always tries to insert the inner html as well.

[saponifi3d]

Hrrmm, looks like that is probably the issue - not this code. Probably just need an if statement there to get the placeholder or call getHtml.

[sarchila]

Okay, lemme try that out, and I can close this and open a PR to rendr-handlebars if it works as intended.

[sarchila]

So, I took your comment as suggesting that rendr-handlebars' getClientPlaceholder function should be invoked on the server to get the outer html placeholder div that is eventually replaced client-side. But, that doesn't work for me, (I get TypeError: Converting circular structure to JSON from this line) and it also appears that getClientPlaceholder is a function that is called client-side, not server-side https://github.com/rendrjs/rendr-handlebars/blob/master/shared/helpers/view.js#L31-L36

[saponifi3d]

k - i mean, there is probably something you can do around that.

Generally, I'm against this as the solution because I could see some cases that you might want to have a lazy loaded view with placeholder stuff, then have it be loaded lazily on the client (you could do that VERY easily with how it's currently setup).

Basically, it seems really weird that the getHtml would return an empty string. I think a bit more investigation into the cause of the bug is in-order.

[sarchila]

Well, as I tried to represent with the tests, getHtml would not return an empty string. It would always return at least the wrapper element (as I understood the intent of the method to do). It just wouldn't have any inner html included if it is a lazy-loaded view.

[saponifi3d]

sorry, wrapper div != placeholder data. i more mean like if you have something loading and you just want to display a placeholder image and some default text, then after you're done fetching you can replace that fake information with the data from the API. With this change it will be impossible to do that, whereas fixing the place it's invoking this function will be a better choice.

it'd probably be better to add a function and allow an override to chose if you just want the wrapper div or a version of the template w/ fake data (how it's working now)

[saponifi3d]

Fyi, master is back to green so rebase off of that. I'm going to remove the peerDepencies sooner rather than later, since npm 3 won't support them anymore.

[saponifi3d]

@sarchila do you have any questions or any issues w/ the approach i was suggesting? (or need any help with it?)

[sarchila]

I think, in my mind, there's still some confusion on the intent of some of these methods.

For instance, I was not aware that, for lazy-loaded views, getHtml was being used to return placeholder inner html that is rendered on the server and later completely replaced on the client-side after the necessary data is fetched.

I feel like it might be more clear if there were a separate server-only view method whose purpose is to return this 'placeholder html' (e.g. 'getPlaceholderHtml') and then in getHtml the line I changed would actually be something like:

var html = this.options.lazy ? this.getPlaceholderHtml() : this.getInnerHtml(),

where getPlaceholderHtml returns an empty string unless overwritten. Thoughts?

[saponifi3d]

the idea i was saying is you can easily set a model with default data and have like a fake version of data to be used as a placeholder. i don't mind having a getPlaceHolderHtml to override, however i don't think we should change what getHtml is going to do like that specifically if the options.lazy is set to true, i think it should be done instead of calling getHtml.

It seems like the bug is in rendr-handlebars rather than the base view. The better fix would be to only call getHtml when we don't have the data we're expecting. Perhaps we have the ability to pass a json blob to a lazy loaded view that can be used to pre-fill the template (an addition to the feature that can come later), and by default just don't invoke the getHtml and instead just return wrapper.

[sarchila]

You think it'd be fine to just replace this line https://github.com/rendrjs/rendr-handlebars/blob/master/shared/helpers/view.js#L55 in rendr-handlebars with something like

return view.options.lazy ? view.getWrapperElementHtml() : view.getHtml();

?

but then I'm thinking that would require writing getWrapperElementHtml in the Rendrjs base view (which I'm imagining would be very similar to the code in getHtml except without the html variable).

[saponifi3d]

i think for your case you can just figure out why the getClientWrapper decided to not work on the server - it should be fairly easy, should probably be changed to getWrapper and then use the state of where it's being called to figure out the inner workings (if required).

[saponifi3d]

@sarchila any progress on this or shall I close the PR?

[sarchila]

It's still an issue that we're encountering, but have chosen to handle it in our instance of the app for now until we can come up with a solution for rendr-handlebars that can handle all of the variables that we discussed.

I'll go ahead and close this in the meantime. 👍