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]

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. :+1: